dohertj2/scadaproj
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(secrets): resolver never re-caches across Invalidate; expired plaintext is evicted (scadaproj#45)

Browse Source 
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
This commit is contained in:
Joseph Doherty
2026-11-09 15:11:22 -05:00
parent eff8c6a00e
commit 16fac72858
2 changed files with 275 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs
+101 -6
View File
@@ -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;
/// <see cref="SecretName.Value"/> and is safe for concurrent use.
/// </para>
/// <para>
/// A resolve that was already reading the store when <see cref="Invalidate"/> 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 <see cref="IDisposable"/> only to stop that sweep timer.
/// </para>
/// <para>
/// A missing or tombstoned secret is <b>not</b> an exception: <see cref="GetAsync"/> returns
/// <c>null</c> and audits a <see cref="AuditOutcome.Failure"/> with a <c>not-found</c> 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.
/// </para>
/// </remarks>
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<string, CacheEntry> _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<string, StrongBox<long>> _generations = new(StringComparer.Ordinal);
private readonly object _timerGate = new();
private ITimer? _sweepTimer;
private bool _disposed;
/// <summary>
/// Creates a resolver over the given store, cipher, and audit writer.
/// </summary>
@@ -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<string, CacheEntry>(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<string, CacheEntry>(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.
/// </summary>
/// <param name="name">The secret whose cache entry to remove.</param>
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 _);
}
/// <summary>Stops the periodic expired-entry sweep. Idempotent.</summary>
public void Dispose()
{
lock (_timerGate)
{
_disposed = true;
_sweepTimer?.Dispose();
_sweepTimer = null;
}
}
/// <summary>Removes every expired entry. Run by the sweep timer; internal for tests.</summary>
internal void SweepExpired()
{
DateTimeOffset now = _timeProvider.GetUtcNow();
foreach (KeyValuePair<string, CacheEntry> pair in _cache)
{
if (now >= pair.Value.ExpiresUtc)
{
_cache.TryRemove(pair);
}
}
}
/// <summary>The number of cached entries (expired or not); internal for tests.</summary>
internal int CachedCount => _cache.Count;
private StrongBox<long> Generation(string key) =>
_generations.GetOrAdd(key, static _ => new StrongBox<long>(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);
ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverCacheTests.cs
+174
View File
@@ -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;
/// <summary>
/// Secrets-006: an <see cref="DefaultSecretResolver.Invalidate"/> 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.
/// </summary>
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,
};
/// <summary>A store whose next read is held until the test releases it.</summary>
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<StoredSecret?> 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<bool> DeleteAsync(SecretName name, string? actor, CancellationToken ct) => throw new NotSupportedException();
public Task<IReadOnlyList<SecretMetadata>> ListAsync(bool includeDeleted, CancellationToken ct) => throw new NotSupportedException();
public Task<IReadOnlyList<SecretManifestEntry>> GetManifestAsync(CancellationToken ct) => throw new NotSupportedException();
public Task ApplyReplicatedAsync(StoredSecret row, CancellationToken ct) => throw new NotSupportedException();
public Task<bool> ApplyRewrapAsync(StoredSecret rewrappedRow, byte[] expectedCurrentWrappedDek, CancellationToken ct) => throw new NotSupportedException();
}
/// <summary>A clock whose timers fire only when the test says so.</summary>
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<string?> 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);
}
}