diff --git a/docs/Configuration.md b/docs/Configuration.md index 66e59fd0..ea007057 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -184,6 +184,8 @@ The checked-in `appsettings*.json` files are deliberately thin: they carry only **Both command-plane gRPC services ride an existing listener — neither claims a port of its own.** `NodeCommandService` is mapped alongside `TelemetryStreamGrpcService` on the driver-role node's `Telemetry:GrpcListenPort` listener (the docker-dev rig's `:4056`) whenever that port is bound, deliberately NOT gated on `MeshTransport:Mode` — mapping it under `Dps` costs nothing (nothing dials it), whereas gating it would mean a running fleet needs a central restart before any node could ack after flipping the mode. `CentralCommandService` (the node→central ack ingress) is mapped alongside `DeploymentArtifactService` on the admin-role node's `ConfigServe:GrpcListenPort` listener (the rig's `:4055`), for the same reason — see [`docs/Telemetry.md`](Telemetry.md) for the shared driver-role listener and [§ `ConfigSource`/`ConfigServe`](#configsource--configserve-config-fetch-and-cache) for the shared admin-role one. +**The transport underneath is cleartext h2c, deliberately.** Both command-plane listeners are HTTP/2-only on a plaintext port and every dial is a prior-knowledge `http://host:port` address, so the PSK bearer crosses the wire in the clear on a link assumed private. That posture is **recorded and accepted** for these two planes and the three others that share their shape — see [`security.md` § Internal gRPC planes (h2c)](security.md#internal-grpc-planes-h2c--accepted-cleartext-posture) for the full five-plane table, what each one exposes, and why a TLS option would have to cover all five at once. + On the docker-dev rig every node carries these keys already; the rig's compose default is `Grpc` (`OTOPCUA_MESH_MODE:-Grpc`, cutover commit `caf2eec6`) — the compiled class default in `MeshTransportOptions` stays `Dps`, so an unconfigured deployment outside the rig still needs `Mode=Grpc` set explicitly. Every rig node carries a `cluster-{ClusterId}` role (Phase 6), so `OTOPCUA_MESH_MODE=Dps` is not a usable override on this rig as shipped — `SplitTopologyTransportValidator` refuses to boot any of them on `Dps`. See the [ClusterClient→gRPC migration design](plans/2026-08-06-clusterclient-to-grpc-design.md), [implementation plan](plans/2026-08-06-clusterclient-to-grpc.md), [tracking doc](plans/2026-08-06-clusterclient-to-grpc-tracking.md) and [live gate](plans/2026-08-06-clusterclient-to-grpc-live-gate.md) for the migration's phase-by-phase history — those are historical records of the cutover and are not updated further by this sweep. diff --git a/docs/security.md b/docs/security.md index c173ace7..db465f21 100644 --- a/docs/security.md +++ b/docs/security.md @@ -104,6 +104,63 @@ The Admin UI `Certificates.razor` page (`src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Co - Promote trusted client certs with the Admin UI Certificates page's **Trust** action (FleetAdmin) — or by moving the `.der` from `rejected/` to `trusted/certs/` — rather than relying on the auto-accept fallback. - Periodically audit the `rejected/` directory; an unexpected entry is often a misconfigured client or a probe attempt. +### Internal gRPC planes (h2c) — accepted cleartext posture + +Everything above concerns the **OPC UA** endpoint — the surface external clients bind to. OtOpcUa +also runs **five internal gRPC planes** between its own processes (central ↔ driver node, and node ↔ +pair partner). Those are a separate transport story, recorded here so it is written down once rather +than rediscovered per plane. + +| # | Plane | gRPC service | Listener (config key) | Credential (config key) | Interceptor | +|---|---|---|---|---|---| +| 1 | Live telemetry stream (central dials the node) | `telemetry.v1.TelemetryStreamService/Subscribe` | driver node, `Telemetry:GrpcListenPort` | `Telemetry:ApiKey` (dialer presents `TelemetryDial:ApiKey`) | `TelemetryStreamAuthInterceptor` | +| 2 | Config-serve / artifact fetch (node fetches from central) | `deployment_artifact.v1.DeploymentArtifactService/Fetch` | central (admin role), `ConfigServe:GrpcListenPort` | `ConfigServe:ApiKey` (fetcher presents `ConfigSource:ApiKey`) | `ConfigServeAuthInterceptor` | +| 3 | LocalDb pair sync (one node dials its partner) | `localdb_sync.v1.LocalDbSync/Sync` | driver node, `LocalDb:SyncListenPort` | `LocalDb:Replication:ApiKey` (byte-identical on both nodes of the pair) | `LocalDbSyncAuthInterceptor` | +| 4 | Mesh commands (central drives the node) | `mesh_transport.v1.NodeCommandService/Execute` | **shares** the node's `Telemetry:GrpcListenPort` listener | `MeshTransport:GrpcPsk` — **per application `Cluster`**, not fleet-wide | `MeshCommandAuthInterceptor` | +| 5 | Mesh deploy acks (node reports to central) | `mesh_transport.v1.CentralCommandService/ReportApply` | **shares** central's `ConfigServe:GrpcListenPort` listener | per-cluster key resolved by `IMeshPskProvider` — `MeshTransport:ClusterPsks:` override, else the `OT-MESH-PSK-` secret | `CentralCommandAuthInterceptor` | + +Planes 4 and 5 claim no port of their own; they ride the listener their sibling already binds. On the +docker-dev rig those two config keys resolve to `:4056` (driver nodes) and `:4055` (central) — rig +values of the keys above, **not** hardcoded ports. + +**The posture.** All five listeners are bound `HttpProtocols.Http2` on a **cleartext** port, and every +client dials a prior-knowledge `http://host:port` address — h2c, no TLS, no ALPN. (Cleartext is +structural, not incidental: a cleartext `Http1AndHttp2` endpoint cannot negotiate HTTP/2 without ALPN, +which is why these are dedicated HTTP/2-only ports rather than a multiplex onto the main one.) +Authentication on all five is a **shared bearer token in an `Authorization` header**, compared in +constant time and fail-closed when unconfigured. Because the transport is cleartext, **the bearer +itself crosses the wire in the clear** — anyone who can observe or interpose on the link can read a +key and then replay it. + +What that buys an attacker is not uniform: + +- Planes 1–2 are **read** surfaces: the live telemetry feed, and the full deployed configuration. +- Plane 3 **writes** rows into a node's deployment-artifact cache — which is what that node boots + from when central SQL is unreachable. +- Planes 4–5 are the **command** surfaces, and are the sharpest: plane 4 carries `RestartDriver`, + `AlarmCommand` and deployment dispatch; plane 5 carries the apply acks the deploy coordinator + counts, so forged acks can seal a deployment nodes never applied. + +**Decision: risk accepted, 2026-08-06** (Gitea [`lmxopcua#545`](https://gitea.dohertylan.com/dohertj2/lmxopcua/issues/545), +option 1). Accepted once, for all five planes together, on the **trusted-link assumption every one of +them already makes**: the central↔site and pair-local links are private (VPN or dedicated WAN), not +routed over anything an untrusted party can observe. Two things bound the consequences if that +assumption is wrong: + +- **Per-cluster PSKs bound blast radius on the two command planes.** A captured `SITE-A` key drives + `SITE-A` only; it is refused by `SITE-B`'s node and by central when it claims to be `SITE-B`. + A single fleet-wide key would have made one observed link equal to fleet-wide command authority. +- **Production keys are delivered as `${secret:...}` references**, never committed literals, and + **MUST NOT be reused across clusters** — reusing one key collapses the isolation above back into a + fleet-wide key with extra steps. The three literal `dev-mesh-psk-*` keys exist only on the + docker-dev rig. + +**Future work.** The recorded alternative is a `UseTls`/scheme option on these planes (dial `https://`, +bind the listener with a certificate). If it is ever built it **must cover all five planes together** +— a mixed posture, where some internal links are encrypted and others are not, is harder to reason +about than either uniform answer, and invites exactly the "which one was it again?" mistake this note +exists to prevent. See issue #545. + --- ## OPC UA Authentication diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/ConfigServeAuthInterceptor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/ConfigServeAuthInterceptor.cs index 91aad7c7..d313506e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/ConfigServeAuthInterceptor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/ConfigServeAuthInterceptor.cs @@ -33,6 +33,11 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; /// wrong key cannot be recovered byte-by-byte from response timing. Length differences are /// unavoidably observable and are not sensitive. /// +/// +/// Exactly one public constructor. Grpc.AspNetCore silently stops invoking an +/// interceptor that has more than one public ctor — see +/// ConfigServeAuthInterceptorTests.HasExactlyOnePublicConstructor. +/// /// public sealed class ConfigServeAuthInterceptor : Interceptor { @@ -110,7 +115,9 @@ public sealed class ConfigServeAuthInterceptor : Interceptor return; var expected = _options.Value.ApiKey; - if (string.IsNullOrEmpty(expected)) + // Whitespace-only counts as unconfigured: an all-blank expected key would otherwise be + // satisfied by an all-blank bearer token, which is no gate at all. + if (string.IsNullOrWhiteSpace(expected)) { _logger.LogWarning( "Rejected a config-serve call to {Method}: no ConfigServe:ApiKey is configured, so the " + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/LocalDbSyncAuthInterceptor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/LocalDbSyncAuthInterceptor.cs index a16acd61..a8b52404 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/LocalDbSyncAuthInterceptor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/LocalDbSyncAuthInterceptor.cs @@ -38,6 +38,11 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; /// a wrong key cannot be recovered byte-by-byte from response timing. Length differences are /// unavoidably observable and are not sensitive. /// +/// +/// Exactly one public constructor. Grpc.AspNetCore silently stops invoking an +/// interceptor that has more than one public ctor — see +/// LocalDbSyncAuthInterceptorTests.HasExactlyOnePublicConstructor. +/// /// public sealed class LocalDbSyncAuthInterceptor : Interceptor { @@ -115,7 +120,9 @@ public sealed class LocalDbSyncAuthInterceptor : Interceptor return; var expected = _options.Value.ApiKey; - if (string.IsNullOrEmpty(expected)) + // Whitespace-only counts as unconfigured: an all-blank expected key would otherwise be + // satisfied by an all-blank bearer token, which is no gate at all. + if (string.IsNullOrWhiteSpace(expected)) { _logger.LogWarning( "Rejected a LocalDb sync call to {Method}: no LocalDb:Replication:ApiKey is configured, " + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/TelemetryStreamAuthInterceptor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/TelemetryStreamAuthInterceptor.cs index 7f331c7b..849cc46e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/TelemetryStreamAuthInterceptor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/TelemetryStreamAuthInterceptor.cs @@ -117,7 +117,9 @@ public sealed class TelemetryStreamAuthInterceptor : Interceptor return; var expected = _options.Value.ApiKey; - if (string.IsNullOrEmpty(expected)) + // Whitespace-only counts as unconfigured: an all-blank expected key would otherwise be + // satisfied by an all-blank bearer token, which is no gate at all. + if (string.IsNullOrWhiteSpace(expected)) { _logger.LogWarning( "Rejected a telemetry stream call to {Method}: no Telemetry:ApiKey is configured, so " + diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/ConfigPublishCoordinatorTimeoutTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/ConfigPublishCoordinatorTimeoutTests.cs index 42fc744b..0490eece 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/ConfigPublishCoordinatorTimeoutTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/ConfigPublishCoordinatorTimeoutTests.cs @@ -22,11 +22,11 @@ public sealed class ConfigPublishCoordinatorTimeoutTests : ControlPlaneActorTest var dbFactory = NewInMemoryDbFactory(); var deploymentId = SeedDispatchingDeployment(dbFactory); - // Short deadline so we can drive it deterministically in the test. - var actor = Sys.ActorOf(ConfigPublishCoordinator.Props(dbFactory, TimeSpan.FromMilliseconds(150))); - - // Seed a NodeDeploymentState row so the coordinator doesn't see "zero expected acks" - // and short-circuit to Sealed. We pretend a driver node exists in the cluster. + // Seed the NodeDeploymentState row BEFORE the actor is spawned. PreStart reads + // NodeDeploymentStates to recover the expected-ack set, and it runs as soon as Sys.ActorOf + // schedules the actor — so seeding afterwards is a race: if PreStart wins, the coordinator + // sees zero expected acks and can short-circuit straight to Sealed, and the assertion below + // never sees TimedOut. We pretend a driver node exists in the cluster. using (var db = dbFactory.CreateDbContext()) { db.NodeDeploymentStates.Add(new Configuration.Entities.NodeDeploymentState @@ -38,6 +38,13 @@ public sealed class ConfigPublishCoordinatorTimeoutTests : ControlPlaneActorTest db.SaveChanges(); } + // Deliberately un-hittable wall-clock deadline (matching the other tests in this class). + // The apply deadline is a real Akka timer: a short one can self-fire under load and race + // the explicit DeadlineElapsed below, so the test would sometimes be measuring the timer + // rather than the message handler. Five minutes guarantees the ONLY deadline signal the + // coordinator ever receives is the one this test sends. + var actor = Sys.ActorOf(ConfigPublishCoordinator.Props(dbFactory, TimeSpan.FromMinutes(5))); + // Drive the deadline ourselves rather than waiting for the cluster's empty driver-set // bypass. Tell the actor the deadline elapsed for this id. actor.Tell(new ConfigPublishCoordinator.DeadlineElapsed(deploymentId)); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ConfigServeAuthInterceptorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ConfigServeAuthInterceptorTests.cs index 333772d4..12f3ac27 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ConfigServeAuthInterceptorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ConfigServeAuthInterceptorTests.cs @@ -65,6 +65,19 @@ public sealed class ConfigServeAuthInterceptorTests ex.StatusCode.ShouldBe(StatusCode.Unauthenticated); } + [Fact] + public async Task FetchMethod_WithBlankKeyConfigured_IsDenied() + { + // Blank is distinct from unset in configuration (a stray-whitespace appsettings value) but + // must be treated identically here: an all-blank expected key would be satisfied by an + // all-blank bearer. Pinned identically on MeshCommandAuthInterceptor. + var interceptor = CreateInterceptor(" "); + var context = CreateContext(FetchMethod, "Bearer "); + + var ex = await Should.ThrowAsync(() => Invoke(interceptor, context)); + ex.StatusCode.ShouldBe(StatusCode.Unauthenticated); + } + [Fact] public async Task FetchMethod_WithNoBearerToken_IsDenied() { @@ -121,6 +134,16 @@ public sealed class ConfigServeAuthInterceptorTests ex.StatusCode.ShouldBe(StatusCode.Unauthenticated); } + [Fact] + public void HasExactlyOnePublicConstructor() + { + // Grpc.AspNetCore silently stops invoking an interceptor with more than one public ctor — + // this pins the shape so a well-meaning overload doesn't quietly disable auth in production. + var publicCtors = typeof(ConfigServeAuthInterceptor).GetConstructors(); + + publicCtors.Length.ShouldBe(1); + } + /// /// Minimal carrying just a method name and request headers — /// the only two things the interceptor reads. Hand-rolled because diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LocalDbSyncAuthInterceptorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LocalDbSyncAuthInterceptorTests.cs index 9a506376..959d52bd 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LocalDbSyncAuthInterceptorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LocalDbSyncAuthInterceptorTests.cs @@ -99,6 +99,19 @@ public sealed class LocalDbSyncAuthInterceptorTests ex.StatusCode.ShouldBe(StatusCode.PermissionDenied); } + [Fact] + public async Task SyncMethod_WithBlankKeyConfigured_IsDenied() + { + // Blank is distinct from unset in configuration (a stray-whitespace appsettings value) but + // must be treated identically here: an all-blank expected key would be satisfied by an + // all-blank bearer. Pinned identically on MeshCommandAuthInterceptor. + var interceptor = CreateInterceptor(" "); + var context = CreateContext(SyncMethod, "Bearer "); + + var ex = await Should.ThrowAsync(() => Invoke(interceptor, context)); + ex.StatusCode.ShouldBe(StatusCode.PermissionDenied); + } + [Fact] public async Task SyncMethod_WithNoBearerToken_IsDenied() { @@ -152,6 +165,16 @@ public sealed class LocalDbSyncAuthInterceptorTests ex.StatusCode.ShouldBe(StatusCode.PermissionDenied); } + [Fact] + public void HasExactlyOnePublicConstructor() + { + // Grpc.AspNetCore silently stops invoking an interceptor with more than one public ctor — + // this pins the shape so a well-meaning overload doesn't quietly disable auth in production. + var publicCtors = typeof(LocalDbSyncAuthInterceptor).GetConstructors(); + + publicCtors.Length.ShouldBe(1); + } + [Fact] public async Task DuplexStreaming_IsGated_BecauseThatIsHowSyncActuallyRuns() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/TelemetryStreamAuthInterceptorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/TelemetryStreamAuthInterceptorTests.cs index 287945d7..811f5f5b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/TelemetryStreamAuthInterceptorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/TelemetryStreamAuthInterceptorTests.cs @@ -65,6 +65,19 @@ public sealed class TelemetryStreamAuthInterceptorTests ex.StatusCode.ShouldBe(StatusCode.PermissionDenied); } + [Fact] + public async Task SubscribeMethod_WithBlankKeyConfigured_IsDenied() + { + // Blank is distinct from unset in configuration (a stray-whitespace appsettings value) but + // must be treated identically here: an all-blank expected key would be satisfied by an + // all-blank bearer. Pinned identically on MeshCommandAuthInterceptor. + var interceptor = CreateInterceptor(" "); + var context = CreateContext(SubscribeMethod, "Bearer "); + + var ex = await Should.ThrowAsync(() => Invoke(interceptor, context)); + ex.StatusCode.ShouldBe(StatusCode.PermissionDenied); + } + [Fact] public async Task SubscribeMethod_WithNoBearerToken_IsDenied() {