fix(secrets): audit decryption failures in resolver (fail-loud, no plaintext)
This commit is contained in:
@@ -6,9 +6,10 @@ namespace ZB.MOM.WW.Secrets;
|
||||
|
||||
/// <summary>
|
||||
/// The default <see cref="ISecretResolver"/>: loads an encrypted row from the store, decrypts it
|
||||
/// on demand, and serves it from a short-lived in-memory cache. Every resolution — hit, miss, or
|
||||
/// tombstone — emits an <see cref="AuditEvent"/> recording <b>which</b> secret was accessed and the
|
||||
/// outcome. The plaintext value is <b>never</b> placed in any audit field.
|
||||
/// on demand, and serves it from a short-lived in-memory cache. Every resolution — hit, miss,
|
||||
/// tombstone, or decryption failure — emits an <see cref="AuditEvent"/> recording <b>which</b>
|
||||
/// secret was accessed and the outcome. The plaintext value is <b>never</b> placed in any audit
|
||||
/// field.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
@@ -23,6 +24,13 @@ namespace ZB.MOM.WW.Secrets;
|
||||
/// <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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// A <b>decryption or integrity failure</b> (tampered ciphertext or the wrong KEK) is different: it
|
||||
/// is <b>fail-loud</b>. <see cref="GetAsync"/> audits a <see cref="AuditOutcome.Failure"/> with a
|
||||
/// <c>decryption-failed</c> action and then rethrows the <see cref="SecretDecryptionException"/> — it
|
||||
/// is never masked as a benign miss or a <c>null</c>. The audit records only the secret name; the
|
||||
/// ciphertext and any value are never included.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class DefaultSecretResolver : ISecretResolver
|
||||
{
|
||||
@@ -30,6 +38,7 @@ public sealed class DefaultSecretResolver : ISecretResolver
|
||||
private const string AuditCategory = "Secrets";
|
||||
private const string ResolveAction = "secret.resolve";
|
||||
private const string NotFoundAction = "secret.resolve.not-found";
|
||||
private const string DecryptionFailedAction = "secret.resolve.decryption-failed";
|
||||
|
||||
private readonly ISecretStore _store;
|
||||
private readonly ISecretCipher _cipher;
|
||||
@@ -94,8 +103,20 @@ public sealed class DefaultSecretResolver : ISecretResolver
|
||||
return null;
|
||||
}
|
||||
|
||||
// Decrypt on demand and cache the plaintext for the short TTL.
|
||||
string plaintext = _cipher.Decrypt(row);
|
||||
// Decrypt on demand and cache the plaintext for the short TTL. A decryption/integrity
|
||||
// failure (tampered ciphertext or wrong KEK) MUST leave an audit trail and then propagate —
|
||||
// it is never masked as a benign miss. The audit records only the name; never the value.
|
||||
string plaintext;
|
||||
try
|
||||
{
|
||||
plaintext = _cipher.Decrypt(row);
|
||||
}
|
||||
catch (SecretDecryptionException)
|
||||
{
|
||||
await AuditDecryptionFailedAsync(name, ct).ConfigureAwait(false);
|
||||
throw;
|
||||
}
|
||||
|
||||
_cache[key] = new CacheEntry(plaintext, now + _cacheTtl, row.Revision);
|
||||
|
||||
await AuditSuccessAsync(name, source: "store", ct).ConfigureAwait(false);
|
||||
@@ -116,6 +137,10 @@ public sealed class DefaultSecretResolver : ISecretResolver
|
||||
private Task AuditNotFoundAsync(SecretName name, CancellationToken ct) =>
|
||||
WriteAuditAsync(name, NotFoundAction, AuditOutcome.Failure, "{\"reason\":\"not-found\"}", ct);
|
||||
|
||||
private Task AuditDecryptionFailedAsync(SecretName name, CancellationToken ct) =>
|
||||
WriteAuditAsync(
|
||||
name, DecryptionFailedAction, AuditOutcome.Failure, "{\"reason\":\"decryption-failed\"}", ct);
|
||||
|
||||
private Task WriteAuditAsync(
|
||||
SecretName name, string action, AuditOutcome outcome, string detailsJson, CancellationToken ct)
|
||||
{
|
||||
|
||||
@@ -118,6 +118,37 @@ public class DefaultSecretResolverTests
|
||||
Assert.Equal(AuditOutcome.Failure, evt.Outcome);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Get_DecryptionFailure_AuditsFailure_AndRethrows()
|
||||
{
|
||||
// The resolver's cipher and the encrypting cipher share a KekId ("k1") but hold different
|
||||
// random key bytes, so the DEK unwrap fails closed with a SecretDecryptionException.
|
||||
var store = new CountingSecretStore();
|
||||
var audit = new CapturingAuditWriter();
|
||||
var clock = new MutableTimeProvider(DateTimeOffset.UnixEpoch);
|
||||
var resolverCipher = new AesGcmEnvelopeCipher(new FakeMasterKeyProvider("k1"));
|
||||
var resolver = new DefaultSecretResolver(store, resolverCipher, audit, Ttl, actorAccessor: null, clock);
|
||||
|
||||
var foreignCipher = new AesGcmEnvelopeCipher(new FakeMasterKeyProvider("k1")); // same id, different key
|
||||
var name = new SecretName("sql/foo");
|
||||
store.Seed(MakeRow(foreignCipher, name, "hunter2"));
|
||||
|
||||
// Fail-loud: the integrity failure must propagate, not be masked as a benign miss/null.
|
||||
await Assert.ThrowsAsync<SecretDecryptionException>(() => resolver.GetAsync(name, CancellationToken.None));
|
||||
|
||||
AuditEvent evt = Assert.Single(audit.Events);
|
||||
Assert.Equal(AuditOutcome.Failure, evt.Outcome);
|
||||
Assert.Equal("secret.resolve.decryption-failed", evt.Action);
|
||||
Assert.Equal("sql/foo", evt.Target);
|
||||
|
||||
// Hard guarantee: the plaintext must NEVER appear in any serialized audit field.
|
||||
foreach (AuditEvent captured in audit.Events)
|
||||
{
|
||||
string json = JsonSerializer.Serialize(captured);
|
||||
Assert.DoesNotContain("hunter2", json, StringComparison.Ordinal);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Invalidate_RemovesCacheEntry()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user