Merge fix/issues-543-544-545: whitespace-key fail-closed backport (#543), de-flaked deadline test (#544), h2c accepted-posture record (#545)
Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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:<clusterId>` override, else the `OT-MESH-PSK-<clusterId>` 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
|
||||
|
||||
@@ -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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Exactly one public constructor.</b> <c>Grpc.AspNetCore</c> silently stops invoking an
|
||||
/// interceptor that has more than one public ctor — see
|
||||
/// <c>ConfigServeAuthInterceptorTests.HasExactlyOnePublicConstructor</c>.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
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 " +
|
||||
|
||||
@@ -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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Exactly one public constructor.</b> <c>Grpc.AspNetCore</c> silently stops invoking an
|
||||
/// interceptor that has more than one public ctor — see
|
||||
/// <c>LocalDbSyncAuthInterceptorTests.HasExactlyOnePublicConstructor</c>.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
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, " +
|
||||
|
||||
@@ -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 " +
|
||||
|
||||
+12
-5
@@ -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));
|
||||
|
||||
+23
@@ -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<RpcException>(() => 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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Minimal <see cref="ServerCallContext"/> carrying just a method name and request headers —
|
||||
/// the only two things the interceptor reads. Hand-rolled because
|
||||
|
||||
+23
@@ -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<RpcException>(() => 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()
|
||||
{
|
||||
|
||||
+13
@@ -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<RpcException>(() => Invoke(interceptor, context));
|
||||
ex.StatusCode.ShouldBe(StatusCode.PermissionDenied);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SubscribeMethod_WithNoBearerToken_IsDenied()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user