From 16fac72858bca2f8a6c7f8829b784640c3e56069 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 15:11:22 -0500 Subject: [PATCH] fix(secrets): resolver never re-caches across Invalidate; expired plaintext is evicted (scadaproj#45) Secrets-006: DefaultSecretResolver keeps a per-key invalidation generation; Invalidate bumps it before evicting, and a resolve that read the store under an older generation withdraws its insert, so a rotation mid-resolve is not undone for another TTL. Expired entries are removed on lookup and by a periodic TimeProvider sweep (every TTL, at least 1 s); a non-positive TTL caches nothing. The resolver is now IDisposable (stops the sweep timer). Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- .../DefaultSecretResolver.cs | 107 ++++++++++- .../DefaultSecretResolverCacheTests.cs | 174 ++++++++++++++++++ 2 files changed, 275 insertions(+), 6 deletions(-) create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverCacheTests.cs diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs index 7152b91..1cfb44e 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs @@ -1,4 +1,5 @@ using System.Collections.Concurrent; +using System.Runtime.CompilerServices; using ZB.MOM.WW.Audit; using ZB.MOM.WW.Secrets.Abstractions; @@ -20,6 +21,14 @@ namespace ZB.MOM.WW.Secrets; /// and is safe for concurrent use. /// /// +/// A resolve that was already reading the store when ran never re-caches +/// what it read: each key carries an invalidation generation, and an entry inserted under a stale +/// generation is withdrawn. Expired entries are removed when they are next looked up and by a +/// periodic sweep (every TTL, at least once a second), so plaintext resolved once — at startup +/// expansion, say — does not stay referenced for the life of the process. A non-positive TTL caches +/// nothing. The resolver is only to stop that sweep timer. +/// +/// /// A missing or tombstoned secret is not an exception: returns /// null and audits a with a not-found action, so a /// consumer can distinguish "absent" from "present" without a throw on the hot path. @@ -32,7 +41,7 @@ namespace ZB.MOM.WW.Secrets; /// ciphertext and any value are never included. /// /// -public sealed class DefaultSecretResolver : ISecretResolver, ISecretCacheInvalidator +public sealed class DefaultSecretResolver : ISecretResolver, ISecretCacheInvalidator, IDisposable { private const string SystemActor = "system"; private const string AuditCategory = "Secrets"; @@ -50,6 +59,14 @@ public sealed class DefaultSecretResolver : ISecretResolver, ISecretCacheInvalid // Keyed by SecretName.Value. Holds DECRYPTED plaintext — kept only for the short TTL. private readonly ConcurrentDictionary _cache = new(StringComparer.Ordinal); + // Per-key invalidation generation; bumped by Invalidate before it evicts. Grows with the set of + // names ever invalidated (bounded by the store), holds no secret material. + private readonly ConcurrentDictionary> _generations = new(StringComparer.Ordinal); + + private readonly object _timerGate = new(); + private ITimer? _sweepTimer; + private bool _disposed; + /// /// Creates a resolver over the given store, cipher, and audit writer. /// @@ -89,12 +106,20 @@ public sealed class DefaultSecretResolver : ISecretResolver, ISecretCacheInvalid DateTimeOffset now = _timeProvider.GetUtcNow(); // 1) Serve from cache while the entry is still live. - if (_cache.TryGetValue(key, out CacheEntry cached) && now < cached.ExpiresUtc) + if (_cache.TryGetValue(key, out CacheEntry cached)) { - await AuditSuccessAsync(name, source: "cache", ct).ConfigureAwait(false); - return cached.Plaintext; + if (now < cached.ExpiresUtc) + { + await AuditSuccessAsync(name, source: "cache", ct).ConfigureAwait(false); + return cached.Plaintext; + } + + // Expired: drop it now (only this exact entry — a concurrent fresh insert survives). + _cache.TryRemove(new KeyValuePair(key, cached)); } + long generation = Volatile.Read(ref Generation(key).Value); + // 2) Load the encrypted row. StoredSecret? row = await _store.GetAsync(name, ct).ConfigureAwait(false); if (row is null || row.IsDeleted) @@ -117,7 +142,23 @@ public sealed class DefaultSecretResolver : ISecretResolver, ISecretCacheInvalid throw; } - _cache[key] = new CacheEntry(plaintext, now + _cacheTtl, row.Revision); + if (_cacheTtl > TimeSpan.Zero) + { + var entry = new CacheEntry(plaintext, now + _cacheTtl, row.Revision); + _cache[key] = entry; + + // Invalidate bumps the generation BEFORE it evicts, so either its eviction runs after + // this insert, or this check sees the new generation: a value read before an + // invalidation is never left in the cache after it. + if (Volatile.Read(ref Generation(key).Value) != generation) + { + _cache.TryRemove(new KeyValuePair(key, entry)); + } + else + { + EnsureSweepTimer(); + } + } await AuditSuccessAsync(name, source: "store", ct).ConfigureAwait(false); return plaintext; @@ -129,7 +170,61 @@ public sealed class DefaultSecretResolver : ISecretResolver, ISecretCacheInvalid /// so a stale value is not served for the remainder of its TTL. Cheap and idempotent. /// /// The secret whose cache entry to remove. - public void Invalidate(SecretName name) => _cache.TryRemove(name.Value, out _); + public void Invalidate(SecretName name) + { + Interlocked.Increment(ref Generation(name.Value).Value); + _cache.TryRemove(name.Value, out _); + } + + /// Stops the periodic expired-entry sweep. Idempotent. + public void Dispose() + { + lock (_timerGate) + { + _disposed = true; + _sweepTimer?.Dispose(); + _sweepTimer = null; + } + } + + /// Removes every expired entry. Run by the sweep timer; internal for tests. + internal void SweepExpired() + { + DateTimeOffset now = _timeProvider.GetUtcNow(); + foreach (KeyValuePair pair in _cache) + { + if (now >= pair.Value.ExpiresUtc) + { + _cache.TryRemove(pair); + } + } + } + + /// The number of cached entries (expired or not); internal for tests. + internal int CachedCount => _cache.Count; + + private StrongBox Generation(string key) => + _generations.GetOrAdd(key, static _ => new StrongBox(0)); + + private void EnsureSweepTimer() + { + if (Volatile.Read(ref _sweepTimer) is not null) + { + return; + } + + lock (_timerGate) + { + if (_sweepTimer is not null || _disposed) + { + return; + } + + TimeSpan period = _cacheTtl < TimeSpan.FromSeconds(1) ? TimeSpan.FromSeconds(1) : _cacheTtl; + _sweepTimer = _timeProvider.CreateTimer( + static state => ((DefaultSecretResolver)state!).SweepExpired(), this, period, period); + } + } private Task AuditSuccessAsync(SecretName name, string source, CancellationToken ct) => WriteAuditAsync(name, ResolveAction, AuditOutcome.Success, $"{{\"source\":\"{source}\"}}", ct); diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverCacheTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverCacheTests.cs new file mode 100644 index 0000000..3056b6a --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverCacheTests.cs @@ -0,0 +1,174 @@ +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Crypto; +using ZB.MOM.WW.Secrets.Tests.Fakes; + +namespace ZB.MOM.WW.Secrets.Tests; + +/// +/// Secrets-006: an that lands while a resolve is +/// reading the store must not be undone by that resolve re-caching the old value, and expired +/// plaintext must not stay referenced for the life of the process. +/// +public class DefaultSecretResolverCacheTests +{ + private static readonly TimeSpan Ttl = TimeSpan.FromSeconds(30); + + private static StoredSecret Row(AesGcmEnvelopeCipher cipher, SecretName name, string value, long revision) => + cipher.Encrypt(name, value, SecretContentType.Text) with + { + Revision = revision, + CreatedUtc = DateTimeOffset.UnixEpoch, + UpdatedUtc = DateTimeOffset.UnixEpoch, + }; + + /// A store whose next read is held until the test releases it. + private sealed class GatedStore : ISecretStore + { + private readonly CountingSecretStore _inner = new(); + + public TaskCompletionSource ReadStarted { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public TaskCompletionSource Release { get; private set; } = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public bool Gate { get; set; } + + public int Reads => _inner.GetCount; + + public void Seed(StoredSecret row) => _inner.Seed(row); + + public async Task GetAsync(SecretName name, CancellationToken ct) + { + StoredSecret? row = await _inner.GetAsync(name, ct); + if (Gate) + { + Gate = false; + ReadStarted.TrySetResult(); + await Release.Task; + } + + return row; + } + + public Task UpsertAsync(StoredSecret row, CancellationToken ct) => throw new NotSupportedException(); + public Task DeleteAsync(SecretName name, string? actor, CancellationToken ct) => throw new NotSupportedException(); + public Task> ListAsync(bool includeDeleted, CancellationToken ct) => throw new NotSupportedException(); + public Task> GetManifestAsync(CancellationToken ct) => throw new NotSupportedException(); + public Task ApplyReplicatedAsync(StoredSecret row, CancellationToken ct) => throw new NotSupportedException(); + public Task ApplyRewrapAsync(StoredSecret rewrappedRow, byte[] expectedCurrentWrappedDek, CancellationToken ct) => throw new NotSupportedException(); + } + + /// A clock whose timers fire only when the test says so. + private sealed class ManualTimerTimeProvider : TimeProvider + { + public DateTimeOffset Now { get; set; } = DateTimeOffset.UnixEpoch; + + public List<(TimerCallback Callback, object? State)> Timers { get; } = new(); + + public override DateTimeOffset GetUtcNow() => Now; + + public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period) + { + Timers.Add((callback, state)); + return new NullTimer(); + } + + public void FireAll() + { + foreach ((TimerCallback callback, object? state) in Timers) + { + callback(state); + } + } + + private sealed class NullTimer : ITimer + { + public bool Change(TimeSpan dueTime, TimeSpan period) => true; + public void Dispose() { } + public ValueTask DisposeAsync() => ValueTask.CompletedTask; + } + } + + [Fact] + public async Task An_invalidate_during_an_in_flight_resolve_is_not_undone_by_it() + { + var cipher = new AesGcmEnvelopeCipher(new FakeMasterKeyProvider("k1")); + var store = new GatedStore(); + var name = new SecretName("db/central"); + store.Seed(Row(cipher, name, "old-password", revision: 3)); + using var resolver = new DefaultSecretResolver(store, cipher, new CapturingAuditWriter(), Ttl); + + // A request thread misses the cache and reads revision 3 ... + store.Gate = true; + Task inFlight = resolver.GetAsync(name, CancellationToken.None); + await store.ReadStarted.Task; + + // ... the operator rotates to revision 4 and the write path invalidates ... + store.Seed(Row(cipher, name, "new-password", revision: 4)); + resolver.Invalidate(name); + + // ... and only then does the request thread finish with the old value. + store.Release.TrySetResult(); + Assert.Equal("old-password", await inFlight); + + // The next resolve must go back to the store, not serve revision 3 for another TTL. + Assert.Equal("new-password", await resolver.GetAsync(name, CancellationToken.None)); + Assert.Equal(2, store.Reads); + } + + [Fact] + public async Task An_expired_entry_is_dropped_when_it_is_looked_up() + { + var cipher = new AesGcmEnvelopeCipher(new FakeMasterKeyProvider("k1")); + var store = new CountingSecretStore(); + var clock = new MutableTimeProvider(DateTimeOffset.UnixEpoch); + var name = new SecretName("db/central"); + store.Seed(Row(cipher, name, "hunter2", 1)); + using var resolver = new DefaultSecretResolver(store, cipher, new CapturingAuditWriter(), Ttl, null, clock); + + await resolver.GetAsync(name, CancellationToken.None); + Assert.Equal(1, resolver.CachedCount); + + // The secret is deleted out-of-band; the expired entry must not outlive the lookup. + store.Seed(Row(cipher, name, "hunter2", 2) with { IsDeleted = true }); + clock.Advance(Ttl + TimeSpan.FromSeconds(1)); + Assert.Null(await resolver.GetAsync(name, CancellationToken.None)); + Assert.Equal(0, resolver.CachedCount); + } + + [Fact] + public async Task The_sweep_drops_expired_plaintext_that_is_never_looked_up_again() + { + var cipher = new AesGcmEnvelopeCipher(new FakeMasterKeyProvider("k1")); + var store = new CountingSecretStore(); + var clock = new ManualTimerTimeProvider(); + var name = new SecretName("db/central"); + store.Seed(Row(cipher, name, "hunter2", 1)); + using var resolver = new DefaultSecretResolver(store, cipher, new CapturingAuditWriter(), Ttl, null, clock); + + await resolver.GetAsync(name, CancellationToken.None); // e.g. startup expansion + Assert.Single(clock.Timers); + + clock.FireAll(); + Assert.Equal(1, resolver.CachedCount); // positive control: not yet expired + + clock.Now += Ttl; + clock.FireAll(); + Assert.Equal(0, resolver.CachedCount); + } + + [Fact] + public async Task A_non_positive_ttl_caches_nothing() + { + var cipher = new AesGcmEnvelopeCipher(new FakeMasterKeyProvider("k1")); + var store = new CountingSecretStore(); + var name = new SecretName("db/central"); + store.Seed(Row(cipher, name, "hunter2", 1)); + using var resolver = new DefaultSecretResolver(store, cipher, new CapturingAuditWriter(), TimeSpan.Zero); + + await resolver.GetAsync(name, CancellationToken.None); + await resolver.GetAsync(name, CancellationToken.None); + + Assert.Equal(0, resolver.CachedCount); + Assert.Equal(2, store.GetCount); + } +}