From 8c4175d8ceeb821a163d05e82f8e0027fd561cc0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 15 Jul 2026 17:01:10 -0400 Subject: [PATCH] fix(secrets): audit decryption failures in resolver (fail-loud, no plaintext) --- .../DefaultSecretResolver.cs | 35 ++++++++++++++++--- .../DefaultSecretResolverTests.cs | 31 ++++++++++++++++ 2 files changed, 61 insertions(+), 5 deletions(-) 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 5325258..6875fc7 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 @@ -6,9 +6,10 @@ namespace ZB.MOM.WW.Secrets; /// /// The default : 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 recording which secret was accessed and the -/// outcome. The plaintext value is never 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 recording which +/// secret was accessed and the outcome. The plaintext value is never placed in any audit +/// field. /// /// /// @@ -23,6 +24,13 @@ namespace ZB.MOM.WW.Secrets; /// null and audits a with a not-found action, so a /// consumer can distinguish "absent" from "present" without a throw on the hot path. /// +/// +/// A decryption or integrity failure (tampered ciphertext or the wrong KEK) is different: it +/// is fail-loud. audits a with a +/// decryption-failed action and then rethrows the — it +/// is never masked as a benign miss or a null. The audit records only the secret name; the +/// ciphertext and any value are never included. +/// /// 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) { diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverTests.cs index e7202c5..631b2fe 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/DefaultSecretResolverTests.cs @@ -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(() => 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() {