fix(mesh): Batch B review fixes — bounded store read, cache poison guard, test hardening

2026-08-06 04:10:03 -04:00
parent 68a416e4fc
commit 24e55d3395
7 changed files with 314 additions and 68 deletions
@@ -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<string, string>? clusterPsks = null)
+        Dictionary<string, string>? clusterPsks = null,
+        ILogger<MeshPskProvider>? 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<MeshPskProvider>.Instance);
+            Options.Create(options), resolver, logger ?? NullLogger<MeshPskProvider>.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<string, string?>
+            {
+                ["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<MeshPskProvider>.Instance);
+
+        provider.TryGetPsk("site-a", out var psk).ShouldBeTrue();
+
+        psk.ShouldBe("rig-psk-site-a");
+        resolver.CallCount.ShouldBe(0);
+    }
+
    /// <summary>
    ///     An <see cref="ISecretResolver"/> 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
        /// <summary>When set, <see cref="GetAsync"/> throws it instead of returning.</summary>
        public Exception? Throw { get; set; }

-        /// <summary>When set, the first call waits on it — used to force a concurrent stampede.</summary>
-        public SemaphoreSlim? Gate { get; set; }
+        /// <summary>
+        ///     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.
+        /// </summary>
+        public TimeSpan ResolveDelay { get; set; } = TimeSpan.Zero;

        /// <summary>How many times the store was consulted.</summary>
        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;
        }
    }
+
+    /// <summary>
+    ///     An <see cref="ILogger{T}"/> 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.
+    /// </summary>
+    private sealed class ThrowingLogger : ILogger<MeshPskProvider>
+    {
+        /// <summary>When true, every <see cref="Log"/> call throws.</summary>
+        public bool ThrowOnLog { get; set; }
+
+        public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
+
+        public bool IsEnabled(LogLevel logLevel) => true;
+
+        public void Log<TState>(
+            LogLevel logLevel,
+            EventId eventId,
+            TState state,
+            Exception? exception,
+            Func<TState, Exception?, string> formatter)
+        {
+            if (ThrowOnLog)
+                throw new InvalidOperationException("logger provider faulted");
+        }
+    }
 }
@@ -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<RpcException>(() => 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<RpcException>(() => Invoke(
+            interceptor, CreateContext(ReportApplyMethod, "Bearer psk-site-a", clusterHeader: null)));
+        var unknownCluster = await Should.ThrowAsync<RpcException>(() => Invoke(
+            interceptor, CreateContext(ReportApplyMethod, "Bearer psk-site-a", "SITE-Z")));
+        var wrongToken = await Should.ThrowAsync<RpcException>(() => Invoke(
+            interceptor, CreateContext(ReportApplyMethod, "Bearer wrong", "SITE-A")));
+        var noToken = await Should.ThrowAsync<RpcException>(() => 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()
    {
@@ -31,10 +31,13 @@ public sealed class MeshCommandAuthInterceptorTests
            NullLogger<MeshCommandAuthInterceptor>.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<RpcException>(() => Invoke(interceptor, context));
+        ex.StatusCode.ShouldBe(StatusCode.PermissionDenied);
+    }
+
    [Fact]
    public async Task ExecuteMethod_TokenComparison_IsNotAPrefixMatch()
    {