diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md index 6352f8f7..12d4c14b 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -781,3 +781,39 @@ Two notes for whoever runs it: cannot resurrect a clock-expired key. 4. New public members added purely so wiring could be pinned by reference: `DataPlaneAuthorizer.Clock`, `BoundedApiKeyStore.Inner` / `.Clock`. + +--- + +## Review outcome — FIX-FIRST, applied 2026-08-20 + +Coordinator verdict on the branch was **FIX-FIRST** with four items. All four applied; Host.Tests and +OpcUaServer.Tests rerun green. + +1. **[Important] `IsExpired` had no test — the review was right, and the gap was mine.** T3's execution + note called the guard a "fail-closed refinement" and shipped it with *no case that fails without it*: + deleting `&& !IsExpired(cached, utcNow)` left all 14 `BoundedApiKeyStoreTests` green. That is the + unchanged-observable trap — the guard's whole effect is invisible unless a cached entry outlives its + key's `ExpiresUtc` **inside** the TTL, which no existing case constructed. + Added **`A_cached_key_that_expires_by_the_clock_is_not_served`**: key expiring at `start + 2 s`, TTL + 60 s, first `FindAsync` non-null (and now cached); flip the inner store to `Revoked`; advance the fake + clock 5 s — past `ExpiresUtc`, far inside the TTL; assert `null` and `inner.Calls == 2`. The + `Revoked` flip is deliberate: it makes "served from cache" and "re-read" differ in the **answer**, not + only in a call count, so a build that re-read and ignored the result cannot pass either. + **RED confirmed by falsification:** with the guard removed, exactly **1 failed / 14 passed** — only the + new case — then restored, 15/15 green. +2. **[Minor] `cache_hit` was metered before the expiry guard could reject.** `TryServe` counted a hit for + an entry that then fell through to a store read, so the counter over-reported. The `Add` moved to the + two call sites, **after** the guard, behind a `MeterCacheHit()` helper; `TryServe` is now purely a + lookup. This matters operationally — `cache_hit` is the ratio §"Bounding the key store" tells an + operator to watch. +3. **[Minor] Added `A_node_with_neither_role_registers_the_deny_all_authenticator`** — asserts the + `IOpcUaUserAuthenticator` singleton **instance** is `NullOpcUaUserAuthenticator.Instance` and that no + `IApiKeyStore` is registered. It closes the third branch of the role conditional, which the guard suite + had left unpinned even though that branch is the one whose absence stops a node booting. +4. **[Minor] `StartRevalidationTimer` now disposes before overwriting.** `StartAsync` can run again when a + host restarts the service around the same instance, and the overwritten field would have left the + previous timer armed with nothing left to dispose it — the dispose-before-overwrite rule the driver + reconnect paths already carry. + +**Verification after the fixes:** solution build 0 errors · Host.Tests **134 passed / 0 failed** (was 132) +· OpcUaServer.Tests **493 passed / 1 skipped**. T11 remains `pending` for the orchestrator. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs index cbf4cf76..bc63006e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs @@ -108,6 +108,10 @@ public sealed class BoundedApiKeyStore : IApiKeyStore && TryServe(_keys, apiKeyId, ttl, out var cached) && !IsExpired(cached, utcNow)) { + // Metered HERE rather than inside TryServe: an entry that is inside the TTL but whose key has + // since expired is not a hit — it falls through to a store read — so counting it in TryServe + // would report cache hits that never happened. + MeterCacheHit(); return cached; } @@ -122,7 +126,11 @@ public sealed class BoundedApiKeyStore : IApiKeyStore public async Task> FindGrantsAsync(string principal, CancellationToken ct) { var ttl = Ttl; - if (ttl > TimeSpan.Zero && TryServe(_grants, principal, ttl, out var cached)) return cached; + if (ttl > TimeSpan.Zero && TryServe(_grants, principal, ttl, out var cached)) + { + MeterCacheHit(); + return cached; + } var fresh = await BoundAsync(token => _inner.FindGrantsAsync(principal, token), ct) .ConfigureAwait(false); @@ -140,6 +148,10 @@ public sealed class BoundedApiKeyStore : IApiKeyStore private static bool IsExpired(ApiKeyLookup? cached, DateTime utcNow) => cached?.Key.ExpiresUtc is { } expires && expires <= utcNow; + /// Counts one lookup the cache actually answered. + private static void MeterCacheHit() => + OtOpcUaTelemetry.ApiKeyStoreBounded.Add(1, new KeyValuePair("outcome", "cache_hit")); + /// Serves an entry if one is present and still inside the TTL. /// The cached value type. /// The cache to read. @@ -152,8 +164,6 @@ public sealed class BoundedApiKeyStore : IApiKeyStore { if (cache.TryGetValue(key, out var entry) && _time.GetUtcNow() - entry.StampedUtc < ttl) { - OtOpcUaTelemetry.ApiKeyStoreBounded.Add( - 1, new KeyValuePair("outcome", "cache_hit")); value = entry.Value; return true; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs index 33a0bb70..adea5467 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs @@ -307,12 +307,19 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl /// /// Separate from so the timer's lifecycle is testable without booting the SDK. /// - internal void StartRevalidationTimer() => + internal void StartRevalidationTimer() + { + // Dispose before overwriting. StartAsync runs again when a host restarts the service around the + // same instance, and an overwritten field would leave the previous timer armed with nothing left + // to dispose it — the same dispose-before-overwrite rule the driver reconnect paths carry. + _revalidationTimer?.Dispose(); + _revalidationTimer = new AclRevalidationTimer( () => _server?.NodeManager?.RevalidateMonitoredItems(), TimeSpan.FromSeconds(_aclOptions.RevalidationIntervalSeconds), _time, _loggerFactory.CreateLogger()); + } /// /// Stops the OPC UA server asynchronously. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs index 1548c3ea..e425a12e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs @@ -76,6 +76,26 @@ public sealed class ApiKeyStoreRegistrationTests sp.GetRequiredService().ShouldBeOfType(); } + /// + /// A node carrying neither role registers the deny-all authenticator explicitly, and no key + /// store at all. + /// + /// + /// Explicitly, not by absence: OtOpcUaServerHostedService takes + /// as a required constructor parameter, and the built-in + /// container does not fall back to a default for an unregistered service — it throws, and the node + /// does not start. The assertion is on the singleton INSTANCE, so a future registration that + /// happened to resolve some other deny-all type still surfaces here. + /// + [Fact] + public void A_node_with_neither_role_registers_the_deny_all_authenticator() + { + using var sp = BuildNode(hasAdmin: false, hasDriver: false); + + sp.GetRequiredService().ShouldBeSameAs(NullOpcUaUserAuthenticator.Instance); + sp.GetService().ShouldBeNull("a node that serves no data plane has no keys to read"); + } + /// Builds the data-plane authentication wiring for one node role combination. /// Whether the node carries the admin role. /// Whether the node carries the driver role. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs index fd521810..9a4357dc 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs @@ -272,6 +272,43 @@ public sealed class BoundedApiKeyStoreTests inner.Calls.ShouldBe(callsAfterFlood + 1); } + /// + /// A cached key whose ExpiresUtc passes inside the TTL is not served: the store is + /// re-read instead. + /// + /// + /// + /// Without this the TTL would resurrect an expired key for up to StoreCacheSeconds — + /// the exact axis 03/S19 exists to close, re-opened by 03/S18's own cache. The cache key is + /// apiKeyId alone (utcNow changes every call and would defeat the cache), so the + /// clock check has to happen on the way OUT of the cache. + /// + /// + /// The inner store is flipped to Revoked first, so "served from cache" and "re-read" + /// give different ANSWERS rather than only different call counts — a build that re-read and + /// then ignored the result could not pass. + /// + /// + [Fact] + public async Task A_cached_key_that_expires_by_the_clock_is_not_served() + { + var start = new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero); + var (_, inner) = SetUp(expiresUtc: start.UtcDateTime.AddSeconds(2)); + var clock = new FakeClock(start); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 60, clock); + + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)) + .ShouldNotBeNull("non-vacuity: the key was usable and IS now cached"); + + inner.Revoked = true; + + // Past ExpiresUtc (+2 s) but far inside the 60 s TTL — the entry is still "fresh" to the cache. + clock.Advance(TimeSpan.FromSeconds(5)); + + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldBeNull(); + inner.Calls.ShouldBe(2, "the expired entry must be re-read, not served"); + } + // ---------- metering ---------- /// A deadline that fires meters outcome=timeout — the alarm signal for a sick ConfigDb. @@ -347,7 +384,7 @@ public sealed class BoundedApiKeyStoreTests Options.Create(new ApiKeyOptions { Username = "apikey", Pepper = Pepper }), NullLogger.Instance); - private static (string Presented, GatedApiKeyStore Store) SetUp() + private static (string Presented, GatedApiKeyStore Store) SetUp(DateTime? expiresUtc = null) { var (presented, apiKeyId, hash) = ApiKeySecret.Generate(Pepper); var key = new ApiKey @@ -358,6 +395,7 @@ public sealed class BoundedApiKeyStoreTests Principal = Principal, Enabled = true, CreatedUtc = DateTime.UtcNow, + ExpiresUtc = expiresUtc, }; return (presented, new GatedApiKeyStore(key)); }