diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/IMeshPskProvider.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/IMeshPskProvider.cs index 61d37f74..a7282694 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/IMeshPskProvider.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/IMeshPskProvider.cs @@ -1,5 +1,7 @@ using System.Collections.Concurrent; using System.Diagnostics.CodeAnalysis; +using System.Security.Cryptography; +using System.Text; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using ZB.MOM.WW.OtOpcUa.Cluster; @@ -21,10 +23,21 @@ namespace ZB.MOM.WW.OtOpcUa.ControlPlane.Communication; /// proceed unauthenticated; a missing key must cost delivery, not authentication. /// /// -/// The Invalidate hooks exist because central's cluster set is DB-dynamic. They are -/// deliberately unconsumed at Phase 0 — Phase 1A wires them to the contact refresh, which is -/// the point at which central learns a cluster's membership (and so possibly its key) has -/// changed. +/// The Invalidate hooks exist because central's cluster set is DB-dynamic. Phase 1A +/// wires them to CentralCommunicationActor's contact refresh, which is the point at +/// which central learns a cluster's membership (and so possibly its key) has changed — so a +/// rotated key is picked up within one ContactRefreshSeconds without a restart. +/// +/// +/// The known-cluster gate is a real security control, not an optimization (Phase 1A +/// security register item 1). publishes the cluster ids +/// central just read from ClusterNode; consults that set +/// first and refuses anything outside it in O(1), touching neither the config map nor the +/// secret store. Without it, an unauthenticated caller can pick an arbitrary +/// x-otopcua-cluster value and make central perform a secret-store read plus an audit +/// write per probe — and, because negatives are deliberately never cached, the cost +/// difference between a known and an unknown cluster is measurable from the wire, which +/// defeats the uniform denial text on the central interceptor. /// /// /// Invalidate is not a revocation barrier. It drops the cached entry; it does @@ -43,6 +56,50 @@ public interface IMeshPskProvider /// true when a usable key was found; otherwise false. bool TryGetPsk(string clusterId, [NotNullWhen(true)] out string? psk); + /// + /// Publishes the set of application Cluster ids central currently knows about. + /// refuses anything outside it before consulting any source. + /// + /// + /// The known cluster ids, matched case-insensitively. An empty collection deactivates the + /// gate rather than refusing everything: central publishes this from its DB contact + /// refresh, and "the DB told me nothing" must not become a fleet-wide authentication outage. + /// Never having called this leaves the gate inactive for the same reason — the central + /// interceptor is live from the first request, before the first refresh has run. + /// + /// + /// Implementations must take a copy: the caller owns its collection and may reuse it. The + /// swap must also be atomic with respect to concurrent callers, + /// which run on arbitrary gRPC threads. + /// + void SetKnownClusters(IReadOnlyCollection clusterIds); + + /// + /// Verifies a presented bearer token against one cluster's key, in constant time with + /// respect to the key's content. + /// + /// The application Cluster the caller claims to be. + /// The bearer token the caller supplied. + /// true only when a key exists for that cluster and the token equals it. + /// + /// + /// This is the only comparison on the verify path (Phase 1A security register + /// item 2). Handing the raw key back so a caller can compare it is how == + /// eventually appears in an interceptor and compiles; keeping the comparison behind this + /// member means the verify side never holds key material at all. + /// survives for the dial side, which has to send the key rather than compare it. + /// + /// + /// Declared without a default implementation on purpose: a security member a new + /// implementer can inherit silently is a member a new implementer never considers. + /// + /// + /// Length differences remain observable through timing; that is inherent to comparing + /// byte arrays of different lengths and is not sensitive. Content is not. + /// + /// + bool Matches(string clusterId, string presented); + /// Drops any cached key for one cluster, so the next lookup re-reads the store. /// The application Cluster id, matched case-insensitively. void Invalidate(string clusterId); @@ -87,6 +144,18 @@ public sealed class MeshPskProvider : IMeshPskProvider private readonly ConcurrentDictionary> _cache = new(StringComparer.OrdinalIgnoreCase); + /// + /// The known-cluster gate's membership set, or when the gate is + /// inactive (never published, or published empty — see ). + /// + /// + /// Swapped wholesale, never mutated in place, so a reader either sees the previous set or the + /// next one and never a half-built one. volatile is what makes the publish visible to + /// the gRPC threads doing the reading; no lock is needed because there is exactly one writer + /// (the contact-refresh handler, on the actor's own thread). + /// + private volatile IReadOnlySet? _knownClusters; + /// Creates the provider. /// Mesh transport options; ClusterPsks is the override map. /// The shared secret store read seam. @@ -118,6 +187,19 @@ public sealed class MeshPskProvider : IMeshPskProvider return false; } + // The known-cluster gate, ahead of BOTH sources. Deliberately silent: the caller that + // rejected the call already logs the denial, and a second line here would be a log flood + // driven by exactly the unauthenticated traffic this gate exists to make cheap. + // + // Ahead of the config map too, not just the store. That has a visible consequence — a + // MeshTransport:ClusterPsks entry for a cluster with no enabled, non-maintenance ClusterNode + // row stops resolving once central publishes its set — and it is the right one: central + // would neither dial such a cluster nor accept an ack from it, so serving a key for it only + // widens what an unauthenticated caller can probe. + var known = _knownClusters; + if (known is not null && !known.Contains(clusterId)) + return false; + // Config map first, on every call. Blank counts as absent: an empty // MeshTransport__ClusterPsks__ is an unset environment variable, not an operator // deciding this cluster has an empty key. @@ -168,6 +250,34 @@ public sealed class MeshPskProvider : IMeshPskProvider return true; } + /// + public bool Matches(string clusterId, string presented) + { + // Blank is refused on its own terms rather than by relying on the fact that a blank key + // never survives resolution: this is an authentication gate, and "both sides happened to be + // empty" must never be a pass. + if (string.IsNullOrWhiteSpace(presented)) + return false; + + if (!TryGetPsk(clusterId, out var expected)) + return false; + + return CryptographicOperations.FixedTimeEquals( + Encoding.UTF8.GetBytes(presented), Encoding.UTF8.GetBytes(expected)); + } + + /// + public void SetKnownClusters(IReadOnlyCollection clusterIds) + { + ArgumentNullException.ThrowIfNull(clusterIds); + + // Copied, so a caller reusing its list cannot widen the gate after the fact; empty collapses + // to null so the gate has exactly one "inactive" representation to test for. + _knownClusters = clusterIds.Count == 0 + ? null + : clusterIds.ToHashSet(StringComparer.OrdinalIgnoreCase); + } + /// public void Invalidate(string clusterId) { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/CentralCommandAuthInterceptor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/CentralCommandAuthInterceptor.cs index 3cedf15f..3b1f635a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/CentralCommandAuthInterceptor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/CentralCommandAuthInterceptor.cs @@ -1,5 +1,3 @@ -using System.Security.Cryptography; -using System.Text; using Grpc.Core; using Grpc.Core.Interceptors; using Microsoft.Extensions.Logging; @@ -38,25 +36,31 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; /// key this design exists to avoid. /// /// -/// Comparison is over UTF-8 bytes, so a -/// wrong key cannot be recovered byte-by-byte from response timing. Length differences are -/// unavoidably observable and are not sensitive. +/// This interceptor never handles a key. Verification is +/// , which owns the constant-time comparison, so a +/// wrong key cannot be recovered byte-by-byte from response timing and no future edit here +/// can replace that comparison with == and still compile (Phase 1A security register +/// item 2). Length differences are unavoidably observable and are not sensitive. /// /// -/// All three denials return the same status text, unlike the sibling interceptors, -/// which distinguish "no key configured" from "authentication failed". Here that distinction -/// would be a content oracle: the caller supplies the cluster id, so a reply that said -/// "no key for that cluster" would let an unauthenticated caller read central's cluster set -/// straight off the wire. The operator-facing detail goes to the log instead. +/// All denials return the same status text, unlike the sibling interceptors, which +/// distinguish "no key configured" from "authentication failed". Here that distinction would +/// be a content oracle: the caller supplies the cluster id, so a reply that said "no +/// key for that cluster" would let an unauthenticated caller read central's cluster set +/// straight off the wire. The operator-facing detail goes to the log instead — and for the +/// same reason the log no longer separates "no key for that cluster" from "wrong key": the +/// provider answers both as one false, and collapsing them costs an operator a hint +/// (restated in the one denial message) rather than costing a diagnosis. /// /// -/// That is not yet full enumeration resistance, and should not be read as such. A -/// timing oracle survives it: an unknown cluster misses the provider's cache and pays a store -/// read on every probe, while a known one is answered from memory after the first. Closing -/// that needs the lookup to be refused before it reaches the store for any cluster central -/// does not know — which is the Phase 1A known-cluster-set gate, where central learns its -/// cluster set from the DB. Until then, uniform text buys content-level indistinguishability -/// only. +/// The timing oracle that survived Phase 0 is closed by the known-cluster gate. An +/// unknown cluster used to miss the provider's cache and pay a secret-store read on every +/// probe, while a known one was answered from memory after the first — a stable, remotely +/// measurable distinguisher, and a pre-auth store/audit flood besides. +/// CentralCommunicationActor now publishes central's cluster set to the provider on +/// every contact refresh, and a cluster outside it is refused in O(1) with no store touch, +/// so the two cases cost the same. What remains is a known cluster's first-ever lookup, +/// which is one read per cluster per process and not attacker-triggerable. /// /// /// Exactly one public constructor. Grpc.AspNetCore silently stops invoking an @@ -151,13 +155,15 @@ public sealed class CentralCommandAuthInterceptor : Interceptor "Central command authentication failed.")); } - if (!_pskProvider.TryGetPsk(clusterId, out var expected)) + // Checked here rather than deferred to Matches so the log can still separate "the caller + // presented nothing" — an operator misconfiguration, and the one case the provider cannot + // see — from "the caller presented something that did not verify". + var presented = GrpcRequestHeaders.BearerToken(context.RequestHeaders); + if (presented is null) { _logger.LogWarning( - "Rejected a central command call to {Method} claiming cluster {ClusterId}: no " - + "pre-shared key could be established for it. Seed the secret " - + "OT-MESH-PSK- on this node, or set " - + "MeshTransport:ClusterPsks: on a rig.", + "Rejected a central command call to {Method} claiming cluster {ClusterId}: no bearer " + + "token presented.", context.Method, clusterId); throw new RpcException(new Status( @@ -165,21 +171,20 @@ public sealed class CentralCommandAuthInterceptor : Interceptor "Central command authentication failed.")); } - var presented = GrpcRequestHeaders.BearerToken(context.RequestHeaders); - if (presented is null || !FixedTimeEquals(presented, expected)) + if (!_pskProvider.Matches(clusterId, presented)) { _logger.LogWarning( - "Rejected a central command call to {Method} claiming cluster {ClusterId}: {Reason}.", + "Rejected a central command call to {Method} claiming cluster {ClusterId}: the " + + "presented bearer token did not match that cluster's pre-shared key, no key could " + + "be established for it, or it is not a cluster this node knows. If the cluster is " + + "legitimate, seed the secret OT-MESH-PSK- on this node, or set " + + "MeshTransport:ClusterPsks: on a rig, and check it has an enabled, " + + "non-maintenance ClusterNode row.", context.Method, - clusterId, - presented is null ? "no bearer token presented" : "bearer token did not match"); + clusterId); throw new RpcException(new Status( StatusCode.PermissionDenied, "Central command authentication failed.")); } } - - private static bool FixedTimeEquals(string presented, string expected) - => CryptographicOperations.FixedTimeEquals( - Encoding.UTF8.GetBytes(presented), Encoding.UTF8.GetBytes(expected)); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Communication/MeshPskProviderTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Communication/MeshPskProviderTests.cs index b87daa32..3360ab9f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Communication/MeshPskProviderTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/Communication/MeshPskProviderTests.cs @@ -367,6 +367,205 @@ public sealed class MeshPskProviderTests resolver.CallCount.ShouldBe(0); } + // ------------------------------------------------------------------ known-cluster gate + // + // Phase 1A security register item 1. Central rebuilds its cluster set from ClusterNode rows on + // every contact refresh; a cluster id outside that set must be refused in O(1) BEFORE anything + // touches the secret store. Two things ride on that: an unauthenticated caller can no longer + // make central perform a store read (and an audit write) per forged x-otopcua-cluster header, + // and the timing difference between a known cluster (cached) and an unknown one (never cached, + // because negatives are deliberately not retained) stops being remotely measurable. + + [Fact] + public void UnknownCluster_WithTheGatePopulated_IsRefused_WithoutTouchingTheStore() + { + // THE assertion of the register item. CallCount == 0 is the whole point: the refusal must + // cost nothing, or the gate is a comment rather than a control. + var resolver = new FakeSecretResolver { Value = "stored-psk" }; + var provider = CreateProvider(resolver); + provider.SetKnownClusters(["SITE-A", "SITE-B"]); + + provider.TryGetPsk("SITE-Z", out var psk).ShouldBeFalse(); + + psk.ShouldBeNull(); + resolver.CallCount.ShouldBe(0); + } + + [Fact] + public void KnownCluster_WithTheGatePopulated_StillResolves() + { + var resolver = new FakeSecretResolver { Value = "stored-psk" }; + var provider = CreateProvider(resolver); + provider.SetKnownClusters(["SITE-A"]); + + provider.TryGetPsk("SITE-A", out var psk).ShouldBeTrue(); + + psk.ShouldBe("stored-psk"); + resolver.CallCount.ShouldBe(1); + } + + [Fact] + public void TheKnownClusterSet_IsCaseInsensitive() + { + // ClusterId case is whatever the operator typed into the DB row; the header is whatever the + // node was configured with. A case-sensitive gate would refuse a legitimate cluster. + var resolver = new FakeSecretResolver { Value = "stored-psk" }; + var provider = CreateProvider(resolver); + provider.SetKnownClusters(["SITE-A"]); + + provider.TryGetPsk("site-a", out var psk).ShouldBeTrue(); + + psk.ShouldBe("stored-psk"); + } + + [Fact] + public void SwappingTheKnownClusterSet_AddsAndRemovesClusters() + { + // The set is rebuilt from the DB every refresh, so both directions happen in production: a + // new site appears, a retired one stops being accepted. + var resolver = new FakeSecretResolver { Value = "stored-psk" }; + var provider = CreateProvider(resolver); + + provider.SetKnownClusters(["SITE-A"]); + provider.TryGetPsk("SITE-A", out _).ShouldBeTrue(); + provider.TryGetPsk("SITE-B", out _).ShouldBeFalse(); + + provider.SetKnownClusters(["SITE-B"]); + provider.TryGetPsk("SITE-B", out _).ShouldBeTrue(); + provider.TryGetPsk("SITE-A", out _).ShouldBeFalse(); + } + + [Fact] + public void AnEmptyKnownClusterSet_LeavesTheGateInactive() + { + // Fail-OPEN, deliberately and narrowly: central publishes the set from its contact refresh, + // and an empty publish means "the DB told me nothing", not "no cluster is legitimate". + // Refusing everything there would make a fleet-wide ack outage the failure mode of an empty + // or unreadable ClusterNode table. + var resolver = new FakeSecretResolver { Value = "stored-psk" }; + var provider = CreateProvider(resolver); + provider.SetKnownClusters(["SITE-A"]); + provider.SetKnownClusters([]); + + provider.TryGetPsk("SITE-Z", out var psk).ShouldBeTrue(); + + psk.ShouldBe("stored-psk"); + } + + [Fact] + public void AnUnsetKnownClusterSet_LeavesTheGateInactive() + { + // Admin boot: the interceptor is live from the first request, but the first contact refresh + // has not run yet. Gating on a set nobody has published would deny every ack in that window. + var resolver = new FakeSecretResolver { Value = "stored-psk" }; + var provider = CreateProvider(resolver); + + provider.TryGetPsk("SITE-Z", out var psk).ShouldBeTrue(); + + psk.ShouldBe("stored-psk"); + } + + [Fact] + public void TheGate_IsConsultedBeforeTheConfigMap() + { + // Consequence worth pinning rather than discovering: a ClusterPsks entry for a cluster with + // no enabled ClusterNode row is refused once the gate is populated. That is correct — central + // would neither dial such a cluster nor accept an ack from it — but it is surprising enough + // on a rig to deserve a test that names it. + var resolver = new FakeSecretResolver(); + var provider = CreateProvider( + resolver, new Dictionary { ["SITE-X"] = "rig-psk-site-x" }); + provider.SetKnownClusters(["SITE-A"]); + + provider.TryGetPsk("SITE-X", out var psk).ShouldBeFalse(); + + psk.ShouldBeNull(); + } + + [Fact] + public void SetKnownClusters_TakesACopy_SoALaterMutationOfTheCallersList_CannotWidenTheGate() + { + var clusters = new List { "SITE-A" }; + var resolver = new FakeSecretResolver { Value = "stored-psk" }; + var provider = CreateProvider(resolver); + provider.SetKnownClusters(clusters); + + clusters.Add("SITE-Z"); + + provider.TryGetPsk("SITE-Z", out _).ShouldBeFalse(); + } + + // ------------------------------------------------------------------ Matches + + [Fact] + public void Matches_AcceptsTheClustersOwnKey() + { + var resolver = new FakeSecretResolver(); + var provider = CreateProvider( + resolver, new Dictionary { ["SITE-A"] = "psk-site-a" }); + + provider.Matches("SITE-A", "psk-site-a").ShouldBeTrue(); + } + + [Fact] + public void Matches_RejectsAnotherClustersKey() + { + // The isolation property, asserted at the provider rather than only at the interceptor: + // choosing a cluster only changes which key you are graded against. + var resolver = new FakeSecretResolver(); + var provider = CreateProvider( + resolver, + new Dictionary { ["SITE-A"] = "psk-site-a", ["SITE-B"] = "psk-site-b" }); + + provider.Matches("SITE-A", "psk-site-b").ShouldBeFalse(); + } + + [Fact] + public void Matches_IsNotAPrefixMatch() + { + var resolver = new FakeSecretResolver(); + var provider = CreateProvider( + resolver, new Dictionary { ["SITE-A"] = "psk-site-a" }); + + provider.Matches("SITE-A", "psk-site-").ShouldBeFalse(); + provider.Matches("SITE-A", "psk-site-a-and-more").ShouldBeFalse(); + } + + [Fact] + public void Matches_FailsClosed_WhenTheClusterHasNoKey() + { + var resolver = new FakeSecretResolver { Value = null }; + var provider = CreateProvider(resolver); + + provider.Matches("SITE-Z", "anything-at-all").ShouldBeFalse(); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void Matches_RejectsABlankPresentedToken(string presented) + { + // A blank key never reaches the cache (blank resolves fail closed), but a blank PRESENTED + // token must be refused on its own terms rather than relying on that. + var resolver = new FakeSecretResolver(); + var provider = CreateProvider( + resolver, new Dictionary { ["SITE-A"] = "psk-site-a" }); + + provider.Matches("SITE-A", presented).ShouldBeFalse(); + } + + [Fact] + public void Matches_IsRefusedByTheKnownClusterGate_WithoutTouchingTheStore() + { + var resolver = new FakeSecretResolver { Value = "stored-psk" }; + var provider = CreateProvider(resolver); + provider.SetKnownClusters(["SITE-A"]); + + provider.Matches("SITE-Z", "stored-psk").ShouldBeFalse(); + + resolver.CallCount.ShouldBe(0); + } + /// /// An whose answer the test controls between calls, so cache /// staleness and invalidation are observable without a real store. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/CentralCommandAuthInterceptorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/CentralCommandAuthInterceptorTests.cs index 026d9030..bb619709 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/CentralCommandAuthInterceptorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/CentralCommandAuthInterceptorTests.cs @@ -242,14 +242,56 @@ public sealed class CentralCommandAuthInterceptorTests publicCtors.Length.ShouldBe(1); } + [Fact] + public async Task Verification_GoesThroughMatches_AndTheInterceptorNeverHandlesTheKeyItself() + { + // Phase 1A security register item 2. The interceptor must not fetch the raw key and compare + // it — that is how a constant-time comparison eventually becomes '=='. Matches owns the + // comparison; TryGetPsk survives only for the DIAL side, which has to send the key. + // (The provider's own default Matches calls TryGetPsk internally; this fake overrides it, so + // a zero TryGetPsk count here means the INTERCEPTOR never asked for the key.) + var provider = new FakeMeshPskProvider(("SITE-A", "psk-site-a")); + var interceptor = new CentralCommandAuthInterceptor( + provider, NullLogger.Instance); + + await Invoke(interceptor, CreateContext(ReportApplyMethod, "Bearer psk-site-a", "SITE-A")); + + provider.MatchesCalls.ShouldBe(1); + provider.TryGetPskCalls.ShouldBe(0); + } + /// An over a fixed key set; unknown clusters fail closed. private sealed class FakeMeshPskProvider(params (string Cluster, string Psk)[] keys) : IMeshPskProvider { private readonly Dictionary _keys = keys.ToDictionary(k => k.Cluster, k => k.Psk, StringComparer.OrdinalIgnoreCase); + /// How many times the raw key was asked for — must stay 0 on the verify path. + public int TryGetPskCalls { get; private set; } + + /// How many times verification went through the provider's comparison. + public int MatchesCalls { get; private set; } + public bool TryGetPsk(string clusterId, [NotNullWhen(true)] out string? psk) - => _keys.TryGetValue(clusterId, out psk); + { + TryGetPskCalls++; + return _keys.TryGetValue(clusterId, out psk); + } + + /// + /// Overrides the interface's default so the two calls are separately countable. The + /// semantics are the default's: no key, or a key that is not exactly the presented token. + /// + public bool Matches(string clusterId, string presented) + { + MatchesCalls++; + return !string.IsNullOrWhiteSpace(presented) + && _keys.TryGetValue(clusterId, out var expected) + && string.Equals(expected, presented, StringComparison.Ordinal); + } + + /// No-op: the gate is the production provider's behaviour, pinned in its own tests. + public void SetKnownClusters(IReadOnlyCollection clusterIds) { } public void Invalidate(string clusterId) { }