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 cbdbc987..61d37f74 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/IMeshPskProvider.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/IMeshPskProvider.cs @@ -26,6 +26,14 @@ namespace ZB.MOM.WW.OtOpcUa.ControlPlane.Communication; /// the point at which central learns a cluster's membership (and so possibly its key) has /// changed. /// +/// +/// Invalidate is not a revocation barrier. It drops the cached entry; it does +/// not wait for, cancel, or supersede a resolve already in flight. A lookup that entered the +/// store before the call can still publish and return the pre-rotation key to its caller, and +/// a lookup racing the eviction can re-cache the old value it had already read. Key rotation +/// is therefore eventually consistent by seconds, not atomic — treat Invalidate as +/// "stop serving this from memory", never as "this key is now dead everywhere". +/// /// public interface IMeshPskProvider { @@ -54,6 +62,14 @@ public sealed class MeshPskProvider : IMeshPskProvider /// public const string SecretNamePrefix = "OT-MESH-PSK-"; + /// + /// Ceiling on one store read. Not a tuning knob — a backstop: the read runs under the + /// publication lock, so a store that never answers would park the + /// calling gRPC thread and every other caller queued behind it for that cluster. + /// Expiry fails closed like any other fault. + /// + private static readonly TimeSpan StoreReadTimeout = TimeSpan.FromSeconds(3); + private readonly IOptions _options; private readonly ISecretResolver _secretResolver; private readonly ILogger _logger; @@ -118,7 +134,27 @@ public sealed class MeshPskProvider : IMeshPskProvider () => self.ResolveFromStore(id), LazyThreadSafetyMode.ExecutionAndPublication), this); - var resolved = entry.Value; + string? resolved; + try + { + resolved = entry.Value; + } + catch + { + // ResolveFromStore swallows everything the STORE can throw — but it logs while doing so, + // and a logger provider that throws escapes the factory. Lazy under + // ExecutionAndPublication caches that exception and rethrows it on every later read, so + // without this guard one bad log call would deny the cluster for the life of the process + // (nothing calls Invalidate until Phase 1A). Evict instance-keyed, same as the negative + // path below, and fail closed. + // + // Deliberately silent: the most likely reason we are here is that logging is what threw, + // so logging the failure is the one action with a real chance of throwing again — out of + // an authentication gate, turning a denial into a 500. + _cache.TryRemove(new KeyValuePair>(clusterId, entry)); + return false; + } + if (string.IsNullOrWhiteSpace(resolved)) { // Never retain a negative. A store that was briefly unreachable would otherwise stay @@ -154,9 +190,12 @@ public sealed class MeshPskProvider : IMeshPskProvider /// /// Blocking on the async resolver is deliberate. The consumer is a gRPC server /// interceptor, whose Authorize is synchronous in all four sibling interceptors, - /// and making this seam async would push that shape onto every one of them. The block - /// happens at most once per cluster per process — every later lookup is a dictionary hit - /// — and the store is a node-local SQLite file, not a network call. + /// and making this seam async would push that shape onto every one of them. What makes + /// the block acceptable is that it is bounded () and + /// happens at most once per cluster per process — every later lookup is a dictionary hit. + /// The bound does not assume anything about what backs the store: this code cannot see + /// whether the resolver reads a local file or crosses a network, so it refuses to wait + /// forever on either. /// /// /// The catch is broad on purpose: this is an authentication boundary, and the set of @@ -184,12 +223,15 @@ public sealed class MeshPskProvider : IMeshPskProvider return null; } + using var cts = new CancellationTokenSource(StoreReadTimeout); try { - return _secretResolver.GetAsync(name, CancellationToken.None).GetAwaiter().GetResult(); + return _secretResolver.GetAsync(name, cts.Token).GetAwaiter().GetResult(); } catch (Exception ex) { + // Covers the timeout too — a cancelled read arrives here as OperationCanceledException + // and is treated as every other fault is: no key. _logger.LogWarning( ex, "Failed to resolve the mesh PSK for cluster {ClusterId} from secret {SecretName}. " 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 367a9e1b..3cedf15f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/CentralCommandAuthInterceptor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/CentralCommandAuthInterceptor.cs @@ -45,9 +45,18 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; /// /// 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 an enumeration oracle: the caller supplies the cluster id, so a distinguishable -/// "no key for that cluster" reply would let an unauthenticated caller map which clusters -/// central holds keys for. The operator-facing detail goes to the log instead. +/// 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. +/// +/// +/// 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. /// /// /// Exactly one public constructor. Grpc.AspNetCore silently stops invoking an @@ -58,9 +67,7 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; public sealed class CentralCommandAuthInterceptor : Interceptor { private const string ServicePrefix = "/mesh_transport.v1.CentralCommandService/"; - private const string AuthorizationHeader = "authorization"; private const string ClusterHeader = "x-otopcua-cluster"; - private const string BearerPrefix = "Bearer "; private readonly IMeshPskProvider _pskProvider; private readonly ILogger _logger; @@ -131,7 +138,7 @@ public sealed class CentralCommandAuthInterceptor : Interceptor if (!context.Method.StartsWith(ServicePrefix, StringComparison.Ordinal)) return; - var clusterId = ExtractHeader(context.RequestHeaders, ClusterHeader); + var clusterId = GrpcRequestHeaders.First(context.RequestHeaders, ClusterHeader); if (string.IsNullOrWhiteSpace(clusterId)) { _logger.LogWarning( @@ -158,7 +165,7 @@ public sealed class CentralCommandAuthInterceptor : Interceptor "Central command authentication failed.")); } - var presented = ExtractBearerToken(context.RequestHeaders); + var presented = GrpcRequestHeaders.BearerToken(context.RequestHeaders); if (presented is null || !FixedTimeEquals(presented, expected)) { _logger.LogWarning( @@ -172,27 +179,6 @@ public sealed class CentralCommandAuthInterceptor : Interceptor } } - private static string? ExtractBearerToken(Metadata headers) - { - var value = ExtractHeader(headers, AuthorizationHeader); - return value is not null && value.StartsWith(BearerPrefix, StringComparison.OrdinalIgnoreCase) - ? value[BearerPrefix.Length..] - : null; - } - - private static string? ExtractHeader(Metadata headers, string name) - { - // gRPC lowercases header keys on the wire; compare case-insensitively anyway so a hand-built - // Metadata in a test behaves the same as a real request. - foreach (var entry in headers) - { - if (string.Equals(entry.Key, name, StringComparison.OrdinalIgnoreCase)) - return entry.Value; - } - - return null; - } - private static bool FixedTimeEquals(string presented, string expected) => CryptographicOperations.FixedTimeEquals( Encoding.UTF8.GetBytes(presented), Encoding.UTF8.GetBytes(expected)); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/GrpcRequestHeaders.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/GrpcRequestHeaders.cs new file mode 100644 index 00000000..77788ce2 --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/GrpcRequestHeaders.cs @@ -0,0 +1,70 @@ +using Grpc.Core; + +namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; + +/// +/// Request-metadata reads shared by the two mesh-command auth interceptors +/// (, ). +/// +/// +/// +/// Exists because those two are documented structural clones of each other, and the one place +/// a clone pair can silently diverge is header parsing — where a difference is not a style +/// difference but a different answer to "which of these two authorization headers +/// counts". They had exactly that divergence before this type: one took the first entry, the +/// other scanned for the first Bearer-prefixed entry, so a request carrying +/// authorization: junk followed by authorization: Bearer <psk> was denied +/// by one and accepted by the other. +/// +/// +/// First entry wins, and it must parse. A non-Bearer first entry is a failed +/// authentication, never a reason to keep looking: scanning past it lets a caller smuggle a +/// credential behind a header some intermediary may have inspected, matched or rewritten, and +/// makes which credential is checked depend on ordering nothing guarantees. +/// +/// +/// The three pre-existing sibling interceptors (LocalDb sync, config-serve, telemetry) keep +/// their own private copies deliberately — they are a shipped, live-gated wire contract, and +/// tightening their header handling is a behaviour change to three running endpoints, not a +/// refactor. +/// +/// +internal static class GrpcRequestHeaders +{ + /// The bearer-token header, lowercased as gRPC puts it on the wire. + internal const string Authorization = "authorization"; + + /// Scheme prefix expected on the value. + internal const string BearerPrefix = "Bearer "; + + /// Gets the first value carried under , or null if absent. + /// The call's request metadata. + /// The header name, matched case-insensitively. + /// The first matching value, or null. + internal static string? First(Metadata headers, string name) + { + // gRPC lowercases header keys on the wire; compare case-insensitively anyway so a hand-built + // Metadata in a test behaves the same as a real request. + foreach (var entry in headers) + { + if (string.Equals(entry.Key, name, StringComparison.OrdinalIgnoreCase)) + return entry.Value; + } + + return null; + } + + /// + /// Gets the token from the first authorization header, or null when that header is + /// absent or does not carry the Bearer scheme. + /// + /// The call's request metadata. + /// The bearer token, or null. + internal static string? BearerToken(Metadata headers) + { + var value = First(headers, Authorization); + return value is not null && value.StartsWith(BearerPrefix, StringComparison.OrdinalIgnoreCase) + ? value[BearerPrefix.Length..] + : null; + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/MeshCommandAuthInterceptor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/MeshCommandAuthInterceptor.cs index b7348d0e..46a93954 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/MeshCommandAuthInterceptor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/MeshCommandAuthInterceptor.cs @@ -43,6 +43,12 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; /// unavoidably observable and are not sensitive. /// /// +/// Header reads go through , shared with +/// — the two are clones, and header parsing is +/// the one place a clone pair can diverge into two different answers about which of several +/// authorization headers counts. +/// +/// /// Exactly one public constructor. Grpc.AspNetCore silently stops invoking an /// interceptor that has more than one public ctor — see /// MeshCommandAuthInterceptorTests.HasExactlyOnePublicConstructor. @@ -51,8 +57,6 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; public sealed class MeshCommandAuthInterceptor : Interceptor { private const string ServicePrefix = "/mesh_transport.v1.NodeCommandService/"; - private const string AuthorizationHeader = "authorization"; - private const string BearerPrefix = "Bearer "; private readonly IOptions _options; private readonly ILogger _logger; @@ -137,7 +141,7 @@ public sealed class MeshCommandAuthInterceptor : Interceptor "Node command service is not accepting calls: no pre-shared key is configured on this node.")); } - var presented = ExtractBearerToken(context.RequestHeaders); + var presented = GrpcRequestHeaders.BearerToken(context.RequestHeaders); if (presented is null || !FixedTimeEquals(presented, expected)) { _logger.LogWarning( @@ -150,23 +154,6 @@ public sealed class MeshCommandAuthInterceptor : Interceptor } } - private static string? ExtractBearerToken(Metadata headers) - { - // gRPC lowercases header keys on the wire; compare case-insensitively anyway so a hand-built - // Metadata in a test behaves the same as a real request. - foreach (var entry in headers) - { - if (!string.Equals(entry.Key, AuthorizationHeader, StringComparison.OrdinalIgnoreCase)) - continue; - - var value = entry.Value; - if (value is not null && value.StartsWith(BearerPrefix, StringComparison.OrdinalIgnoreCase)) - return value[BearerPrefix.Length..]; - } - - return null; - } - 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 bfc4fc05..71b7f887 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 @@ -1,3 +1,5 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using Shouldly; @@ -22,7 +24,8 @@ public sealed class MeshPskProviderTests { private static MeshPskProvider CreateProvider( ISecretResolver resolver, - Dictionary? clusterPsks = null) + Dictionary? clusterPsks = null, + ILogger? logger = null) { var options = new MeshTransportOptions(); if (clusterPsks is not null) @@ -32,7 +35,7 @@ public sealed class MeshPskProviderTests } return new MeshPskProvider( - Options.Create(options), resolver, NullLogger.Instance); + Options.Create(options), resolver, logger ?? NullLogger.Instance); } [Fact] @@ -275,28 +278,92 @@ public sealed class MeshPskProviderTests resolver.CallCount.ShouldBe(0); } - [Fact] + [Fact(Timeout = 30_000)] public async Task ConcurrentCallers_ResolveOnce_AndAllSeeTheSameValue() { // The gRPC gate calls this from every inbound ack, on arbitrary thread-pool threads. A // non-atomic fill would stampede the store and could hand two callers different keys. - var gate = new SemaphoreSlim(0, 1); - var resolver = new FakeSecretResolver { Value = "stored-psk", Gate = gate }; + // + // The shape matters more than the assertion. Callers park BEFORE TryGetPsk and are released + // together, so the race is real rather than accidentally serialized by task scheduling — + // parking them INSIDE the resolver could never work, because the whole point of the Lazy is + // that only one caller ever gets in there. And the resolver's delay RETURNS rather than + // waiting on a gate: a regression that lets a second caller reach the store must drive + // CallCount to 2 (red), never sit on an exhausted semaphore (hang, which reads as a flake). + const int Callers = 16; + using var arrived = new CountdownEvent(Callers); + using var start = new ManualResetEventSlim(false); + var resolver = new FakeSecretResolver { Value = "stored-psk", ResolveDelay = TimeSpan.FromMilliseconds(50) }; var provider = CreateProvider(resolver); - var callers = Enumerable.Range(0, 16).Select(_ => Task.Run(() => + var callers = Enumerable.Range(0, Callers).Select(_ => Task.Run(() => { + arrived.Signal(); + start.Wait(); provider.TryGetPsk("SITE-A", out var psk).ShouldBeTrue(); return psk; })).ToArray(); - gate.Release(); + arrived.Wait(TimeSpan.FromSeconds(10)).ShouldBeTrue("all callers should reach the start gate"); + start.Set(); var results = await Task.WhenAll(callers); results.ShouldAllBe(r => r == "stored-psk"); resolver.CallCount.ShouldBe(1); } + [Fact] + public void APoisonedCacheEntry_IsEvicted_RatherThanDenyingTheClusterForever() + { + // ResolveFromStore swallows what the store throws, but it LOGS while doing so — and a logger + // provider that throws escapes the factory. Lazy under ExecutionAndPublication caches that + // exception and rethrows it on every later read, so an unguarded entry.Value would deny this + // cluster for the life of the process; nothing calls Invalidate until Phase 1A. + var logger = new ThrowingLogger { ThrowOnLog = true }; + var resolver = new FakeSecretResolver { Throw = new InvalidOperationException("KEK unavailable") }; + var provider = CreateProvider(resolver, logger: logger); + + // The logger throwing must not escape the gate either — a denial, not a 500. + provider.TryGetPsk("SITE-A", out var first).ShouldBeFalse(); + first.ShouldBeNull(); + resolver.CallCount.ShouldBe(1); + + logger.ThrowOnLog = false; + resolver.Throw = null; + resolver.Value = "stored-psk"; + + provider.TryGetPsk("SITE-A", out var second).ShouldBeTrue(); + second.ShouldBe("stored-psk"); + resolver.CallCount.ShouldBe(2); // reached the store again — the entry was not poisoned + } + + [Fact] + public void ConfigMapLookup_StaysCaseInsensitive_ThroughRealConfigurationBinding() + { + // Writing into the options object pins only the field initializer's comparer. What reaches + // production is a BOUND instance, and the binder is free to construct its own Dictionary and + // drop the comparer silently — the symptom being a rig lookup that misses on a ClusterId + // spelled the way the DB spells it. Bind for real, then go through the PROVIDER. + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["MeshTransport:ClusterPsks:SITE-A"] = "rig-psk-site-a", + }) + .Build(); + + var options = new MeshTransportOptions(); + configuration.GetSection(MeshTransportOptions.SectionName).Bind(options); + + var resolver = new FakeSecretResolver(); + var provider = new MeshPskProvider( + Options.Create(options), resolver, NullLogger.Instance); + + provider.TryGetPsk("site-a", out var psk).ShouldBeTrue(); + + psk.ShouldBe("rig-psk-site-a"); + resolver.CallCount.ShouldBe(0); + } + /// /// An whose answer the test controls between calls, so cache /// staleness and invalidation are observable without a real store. @@ -311,8 +378,12 @@ public sealed class MeshPskProviderTests /// When set, throws it instead of returning. public Exception? Throw { get; set; } - /// When set, the first call waits on it — used to force a concurrent stampede. - public SemaphoreSlim? Gate { get; set; } + /// + /// How long each call takes before answering. Widens the window a concurrent stampede + /// would have to exploit — and it RETURNS, so a second entrant shows up as a second call + /// rather than as a hang. + /// + public TimeSpan ResolveDelay { get; set; } = TimeSpan.Zero; /// How many times the store was consulted. public int CallCount => Volatile.Read(ref _callCount); @@ -325,8 +396,8 @@ public sealed class MeshPskProviderTests LastName = name.Value; Interlocked.Increment(ref _callCount); - if (Gate is not null) - await Gate.WaitAsync(ct).ConfigureAwait(false); + if (ResolveDelay > TimeSpan.Zero) + await Task.Delay(ResolveDelay, ct).ConfigureAwait(false); if (Throw is not null) throw Throw; @@ -334,4 +405,29 @@ public sealed class MeshPskProviderTests return Value; } } + + /// + /// An that can be made to throw, standing in for a logger provider + /// that faults — the only way the provider's own catch blocks can throw. + /// + private sealed class ThrowingLogger : ILogger + { + /// When true, every call throws. + public bool ThrowOnLog { get; set; } + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + if (ThrowOnLog) + throw new InvalidOperationException("logger provider faulted"); + } + } } 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 ec4a30f4..026d9030 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 @@ -30,10 +30,17 @@ public sealed class CentralCommandAuthInterceptorTests private static ServerCallContext CreateContext( string method, string? authorizationHeader, string? clusterHeader) + => CreateContext( + method, + authorizationHeader is null ? [] : [authorizationHeader], + clusterHeader); + + private static ServerCallContext CreateContext( + string method, string[] authorizationHeaders, string? clusterHeader) { var headers = new Metadata(); - if (authorizationHeader is not null) - headers.Add("authorization", authorizationHeader); + foreach (var value in authorizationHeaders) + headers.Add("authorization", value); if (clusterHeader is not null) headers.Add("x-otopcua-cluster", clusterHeader); @@ -143,6 +150,47 @@ public sealed class CentralCommandAuthInterceptorTests ex.StatusCode.ShouldBe(StatusCode.PermissionDenied); } + [Fact] + public async Task ReportApply_WithAJunkAuthorizationHeaderBeforeTheRealOne_IsDenied() + { + // First entry wins, and it must parse. Scanning past a non-Bearer header for a later one + // would let a caller smuggle its credential behind a header an intermediary may have + // inspected or rewritten, and would make WHICH credential is checked depend on ordering + // nothing guarantees. Pinned identically on MeshCommandAuthInterceptor. + var interceptor = CreateInterceptor(("SITE-A", "psk-site-a")); + var context = CreateContext( + ReportApplyMethod, ["junk", "Bearer psk-site-a"], "SITE-A"); + + var ex = await Should.ThrowAsync(() => Invoke(interceptor, context)); + ex.StatusCode.ShouldBe(StatusCode.PermissionDenied); + } + + [Fact] + public async Task EveryDenialArm_ReturnsAnIdenticalDetail() + { + // The anti-enumeration mechanism IS this identity: the caller supplies the cluster id, so a + // reply distinguishing "no key for that cluster" from "wrong key" would let an + // unauthenticated caller read central's cluster set off the wire. Asserting only StatusCode + // would let a well-meaning "more helpful message" edit break it silently. + // (Content-level only — the class remark records the timing oracle that survives until the + // Phase 1A known-cluster-set gate.) + var interceptor = CreateInterceptor(("SITE-A", "psk-site-a")); + + var noHeader = await Should.ThrowAsync(() => Invoke( + interceptor, CreateContext(ReportApplyMethod, "Bearer psk-site-a", clusterHeader: null))); + var unknownCluster = await Should.ThrowAsync(() => Invoke( + interceptor, CreateContext(ReportApplyMethod, "Bearer psk-site-a", "SITE-Z"))); + var wrongToken = await Should.ThrowAsync(() => Invoke( + interceptor, CreateContext(ReportApplyMethod, "Bearer wrong", "SITE-A"))); + var noToken = await Should.ThrowAsync(() => Invoke( + interceptor, CreateContext(ReportApplyMethod, authorizationHeader: null, clusterHeader: "SITE-A"))); + + unknownCluster.Status.Detail.ShouldBe(noHeader.Status.Detail); + wrongToken.Status.Detail.ShouldBe(noHeader.Status.Detail); + noToken.Status.Detail.ShouldBe(noHeader.Status.Detail); + noHeader.Status.Detail.ShouldNotBeNullOrWhiteSpace(); + } + [Fact] public async Task ServerStreamingPath_IsAlsoGated() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshCommandAuthInterceptorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshCommandAuthInterceptorTests.cs index d2446bbb..b96e28b0 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshCommandAuthInterceptorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshCommandAuthInterceptorTests.cs @@ -31,10 +31,13 @@ public sealed class MeshCommandAuthInterceptorTests NullLogger.Instance); private static ServerCallContext CreateContext(string method, string? authorizationHeader) + => CreateContext(method, authorizationHeader is null ? [] : [authorizationHeader]); + + private static ServerCallContext CreateContext(string method, string[] authorizationHeaders) { var headers = new Metadata(); - if (authorizationHeader is not null) - headers.Add("authorization", authorizationHeader); + foreach (var value in authorizationHeaders) + headers.Add("authorization", value); return new FakeServerCallContext(method, headers); } @@ -133,6 +136,20 @@ public sealed class MeshCommandAuthInterceptorTests await Invoke(interceptor, context); // no throw } + [Fact] + public async Task ExecuteMethod_WithAJunkAuthorizationHeaderBeforeTheRealOne_IsDenied() + { + // First entry wins, and it must parse. This interceptor originally scanned past a non-Bearer + // header for a later Bearer one while its central-side clone took the first entry, so the + // two clones gave OPPOSITE answers to this exact request. Both now share + // GrpcRequestHeaders and both deny it; pinned identically on CentralCommandAuthInterceptor. + var interceptor = CreateInterceptor("the-cluster-psk"); + var context = CreateContext(ExecuteMethod, ["junk", "Bearer the-cluster-psk"]); + + var ex = await Should.ThrowAsync(() => Invoke(interceptor, context)); + ex.StatusCode.ShouldBe(StatusCode.PermissionDenied); + } + [Fact] public async Task ExecuteMethod_TokenComparison_IsNotAPrefixMatch() {