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 48b81e82..7689c090 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -618,3 +618,17 @@ for the orchestrator. `Anonymous_grant_resolution_is_bounded_too` (moved forward from the plan's T3 list — the deadline half reaches `FindGrantsAsync` in the same change, so it is asserted in the same change). - 5 tests, green. + +### T3 — TTL read-through cache + the falsification control + +- **RED observed:** `error CS0117: 'BoundedApiKeyStore' does not contain a definition for + 'MaxCacheEntries'` — the cache and its ceiling absent. +- **DEVIATION (fail-closed refinement, 2 lines):** the plan keys `FindAsync`'s cache on `apiKeyId` alone. + Taken literally that lets the cache **resurrect a key that expired by the clock** for up to a TTL, which + is exactly the axis 03/S19 exists to close. A cached hit is therefore re-checked against the caller's + `utcNow` before it is served (`IsExpired`) — so the TTL bounds how stale an `Enabled` **revocation** may + be, and nothing else. `utcNow` is still not part of the key (it changes every call and would defeat the + cache entirely). +- The counter's `cache_hit` / `timeout` `Add` calls are deliberately NOT here — they are T4, and writing + them early would have made T4's red vacuous. +- 11 tests in the class, green. diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json index f7855020..cf158b9e 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json @@ -19,7 +19,7 @@ { "id": "T3", "subject": "BoundedApiKeyStore: short TTL read-through cache + falsification control (a revoked key is denied once the TTL has passed)", - "status": "pending", + "status": "done", "blockedBy": [ "T2" ] 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 cd085541..63c03710 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using ZB.MOM.WW.OtOpcUa.Configuration.Entities; @@ -24,13 +25,34 @@ namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; /// a bad credential. Nothing here can admit a session the inner store did not authorize — the /// decorator can only return what the inner store returned, or throw. /// +/// +/// ⚠️ Faults and timeouts are NEVER cached. Caching a deny would be fail-closed and is +/// still wrong: it would extend a ConfigDb outage past the outage, denying valid keys for a +/// further TTL after the database came back. Only a COMPLETED lookup — a hit or an authoritative +/// miss — is stored. +/// /// public sealed class BoundedApiKeyStore : IApiKeyStore { + /// + /// Entries either cache may hold before it is cleared wholesale. + /// + /// + /// Wholesale rather than per-entry eviction, for the reason SessionAclDecisionCache already + /// documents: picking WHICH entry to drop needs knowledge the cache does not have, and a + /// repopulate is cheap and cannot be wrong. The ceiling exists so a key-id guessing flood is a + /// bounded memory event — negative results are cached deliberately, so that a flood of unknown ids + /// does not become a SQL flood. + /// + public const int MaxCacheEntries = 1024; + private readonly IApiKeyStore _inner; private readonly IOptions _options; private readonly ILogger _log; private readonly TimeProvider _time; + private readonly ConcurrentDictionary> _keys = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary>> _grants = + new(StringComparer.Ordinal); /// Initializes a new instance of the class. /// The store being bounded. @@ -59,12 +81,88 @@ public sealed class BoundedApiKeyStore : IApiKeyStore } /// - public Task FindAsync(string apiKeyId, DateTime utcNow, CancellationToken ct) => - BoundAsync(token => _inner.FindAsync(apiKeyId, utcNow, token), ct); + /// + /// Keyed by alone. is deliberately not part + /// of the key — it changes on every call, so including it would defeat the cache entirely — but a + /// cached hit IS re-checked against it before being served, so the TTL can never resurrect a key + /// that has expired by the clock in the meantime. The TTL therefore bounds how stale an + /// Enabled revocation may be, and nothing else. + /// + public async Task FindAsync(string apiKeyId, DateTime utcNow, CancellationToken ct) + { + var ttl = Ttl; + if (ttl > TimeSpan.Zero + && TryServe(_keys, apiKeyId, ttl, out var cached) + && !IsExpired(cached, utcNow)) + { + return cached; + } + + var fresh = await BoundAsync(token => _inner.FindAsync(apiKeyId, utcNow, token), ct) + .ConfigureAwait(false); + if (ttl > TimeSpan.Zero) Store(_keys, apiKeyId, fresh); + return fresh; + } /// - public Task> FindGrantsAsync(string principal, CancellationToken ct) => - BoundAsync(token => _inner.FindGrantsAsync(principal, token), ct); + /// Keyed by principal — the anonymous activation path, which runs on every unauthenticated connect. + public async Task> FindGrantsAsync(string principal, CancellationToken ct) + { + var ttl = Ttl; + if (ttl > TimeSpan.Zero && TryServe(_grants, principal, ttl, out var cached)) return cached; + + var fresh = await BoundAsync(token => _inner.FindGrantsAsync(principal, token), ct) + .ConfigureAwait(false); + if (ttl > TimeSpan.Zero) Store(_grants, principal, fresh); + return fresh; + } + + /// The configured TTL; non-positive means the cache is off. + private TimeSpan Ttl => TimeSpan.FromSeconds(_options.Value.StoreCacheSeconds); + + /// Whether a cached lookup has since expired by the clock. + /// The cached lookup, possibly null (an authoritative miss). + /// The caller's current time. + /// when the cached key must not be served. + private static bool IsExpired(ApiKeyLookup? cached, DateTime utcNow) => + cached?.Key.ExpiresUtc is { } expires && expires <= utcNow; + + /// Serves an entry if one is present and still inside the TTL. + /// The cached value type. + /// The cache to read. + /// The cache key. + /// How long an entry stays servable. + /// The cached value, when this returns true. + /// on a hit. + private bool TryServe( + ConcurrentDictionary> cache, string key, TimeSpan ttl, out T value) + { + if (cache.TryGetValue(key, out var entry) && _time.GetUtcNow() - entry.StampedUtc < ttl) + { + value = entry.Value; + return true; + } + + value = default!; + return false; + } + + /// Stores a COMPLETED lookup, clearing wholesale first if the ceiling has been reached. + /// The cached value type. + /// The cache to write. + /// The cache key. + /// The value to cache. + private void Store(ConcurrentDictionary> cache, string key, T value) + { + if (cache.Count >= MaxCacheEntries) cache.Clear(); + cache[key] = new Entry(_time.GetUtcNow(), value); + } + + /// One cached lookup and the instant it was taken. + /// The cached value type. + /// When the inner store answered. + /// What it answered. + private readonly record struct Entry(DateTimeOffset StampedUtc, T Value); /// Runs one inner call under the configured deadline. /// The inner call's result type. 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 f448aa4c..80a7fa4c 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 @@ -5,6 +5,7 @@ using Xunit; using ZB.MOM.WW.OtOpcUa.Configuration.Entities; using ZB.MOM.WW.OtOpcUa.Core.Authorization; using ZB.MOM.WW.OtOpcUa.Host.OpcUa; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa; @@ -138,6 +139,137 @@ public sealed class BoundedApiKeyStoreTests result.Success.ShouldBeFalse(); } + // ---------- TTL read-through cache ---------- + + /// A second activation inside the TTL is served from the cache and never reaches the store. + [Fact] + public async Task A_second_activation_within_the_TTL_does_not_reach_the_store() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 5, clock); + + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + clock.Advance(TimeSpan.FromSeconds(2)); + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + + inner.Calls.ShouldBe(1); + } + + /// + /// FALSIFICATION CONTROL. A key revoked in the store is denied once the TTL has passed — the + /// cache expires rather than merely existing. + /// + /// + /// A cache with no expiry at all passes every other case in this class: the hit test, the + /// read-through test and the fault test are all satisfied by "store once, serve forever". This is + /// the one that makes StoreCacheSeconds a bound rather than a claim. + /// + [Fact] + public async Task A_revoked_key_is_denied_once_the_TTL_has_passed() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 5, clock); + + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)) + .ShouldNotBeNull("non-vacuity: the key really was usable before the revocation"); + + inner.Revoked = true; + + // Still inside the TTL — the documented, accepted staleness window. + clock.Advance(TimeSpan.FromSeconds(3)); + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + + // Past it — the store is re-read and the revocation lands. + clock.Advance(TimeSpan.FromSeconds(3)); + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldBeNull(); + inner.Calls.ShouldBe(2); + } + + /// A non-positive TTL disables the cache: every call reads through, today's behaviour. + [Fact] + public async Task TTL_zero_reads_through_every_time() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 0, clock); + + for (var i = 0; i < 3; i++) + await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default); + + inner.Calls.ShouldBe(3); + } + + /// + /// A fault is never cached: the next call reaches the store, so an outage cannot outlive itself. + /// + /// + /// Caching a deny would be fail-closed and is still wrong — it would go on denying valid keys for + /// a further TTL after the database came back. + /// + [Fact] + public async Task A_faulted_store_is_not_cached() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 60, clock); + inner.ThrowsRemaining = 1; + + await Should.ThrowAsync( + () => sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)); + + // Same instant — a cached fault would still be "fresh" here, so the clock cannot rescue this. + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + inner.Calls.ShouldBe(2); + } + + /// The anonymous grant lookup is cached on the same terms, keyed by principal. + [Fact] + public async Task Anonymous_grants_are_cached_too() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 5, clock); + + await sut.FindGrantsAsync(OpcUaPrincipals.Anonymous, default); + await sut.FindGrantsAsync(OpcUaPrincipals.Anonymous, default); + inner.Calls.ShouldBe(1); + + clock.Advance(TimeSpan.FromSeconds(6)); + await sut.FindGrantsAsync(OpcUaPrincipals.Anonymous, default); + inner.Calls.ShouldBe(2); + } + + /// + /// Past the entry ceiling the cache clears wholesale and keeps answering — a key-id guessing flood + /// is a bounded memory event, not an unbounded one. + /// + /// + /// Wholesale rather than per-entry eviction for the reason SessionAclDecisionCache already + /// documents: choosing WHICH entry to drop needs knowledge the cache does not have, and a + /// repopulate is cheap and cannot be wrong. + /// + [Fact] + public async Task Cache_overflow_clears_wholesale_and_keeps_serving() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 600, clock); + + // Seed the real key, then flood past the ceiling with ids that do not exist. + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + for (var i = 0; i < BoundedApiKeyStore.MaxCacheEntries + 1; i++) + await sut.FindAsync($"ghost-{i}", clock.GetUtcNow().UtcDateTime, default); + + var callsAfterFlood = inner.Calls; + + // Still correct after the clear — and the real key was evicted, so it is re-read rather than + // served from a cache the flood was supposed to have emptied. + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + inner.Calls.ShouldBe(callsAfterFlood + 1); + } + // ---------- helpers ---------- private static BoundedApiKeyStore Bounded( @@ -179,6 +311,9 @@ public sealed class BoundedApiKeyStoreTests /// private sealed class GatedApiKeyStore(ApiKey key) : IApiKeyStore { + /// The id of the key this store holds. + public string KeyId => key.ApiKeyId; + /// When set, every call awaits this before answering. public Task? Gate { get; set; }