fix(secrets): malformed rows fail as SecretDecryptionException; one KEK snapshot per cipher op (scadaproj#45)

Secrets-004: AesGcmEnvelopeCipher.Decrypt now wraps the ArgumentException AesGcm throws for a wrong-length nonce/tag/wrapped DEK as SecretDecryptionException (as Rewrap already did), so the resolver's decryption-failed audit fires. Secrets-007: new MasterKeySnapshot (key + KekId from one reading, zeroed on Dispose) and a default-implemented IMasterKeyProvider.GetSnapshot(); MasterKeyProviderBase overrides it to resolve the source once and derive the id from that same key. Encrypt/Decrypt/Rewrap take one snapshot per KEK and dispose it, so a key file swapped mid-operation can no longer stamp a row with one key's id and wrap it under another. Intermediate key buffers are zeroed. On-disk format unchanged: a golden row written by the 0.6.5 cipher still decrypts. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-09 15:06:57 -05:00
parent b33f0f93c9
commit b64628de0b
7 changed files with 359 additions and 14 deletions
@@ -21,4 +21,18 @@ public interface IMasterKeyProvider
    /// to the KEK that wrapped their DEK.
    /// </summary>
    string KekId { get; }
+
+    /// <summary>
+    /// Returns the key and its <see cref="KekId"/> as one consistent, caller-owned snapshot. The
+    /// cipher takes exactly one snapshot per operation and disposes it (zeroing its key copy).
+    /// </summary>
+    /// <remarks>
+    /// The default implementation reads <see cref="KekId"/> and then copies
+    /// <see cref="GetMasterKey"/>; a provider whose source can change between those two reads (a
+    /// file or environment variable) should override it to resolve the key once and derive the id
+    /// from that same reading, as <c>MasterKeyProviderBase</c> does.
+    /// </remarks>
+    /// <returns>A snapshot the caller must dispose.</returns>
+    /// <exception cref="MasterKeyUnavailableException">The master key cannot be resolved.</exception>
+    MasterKeySnapshot GetSnapshot() => new(KekId, GetMasterKey().ToArray());
 }
@@ -0,0 +1,41 @@
+using System.Security.Cryptography;
+
+namespace ZB.MOM.WW.Secrets.Abstractions;
+
+/// <summary>
+/// One consistent reading of a master key (KEK): the 32 key bytes together with the
+/// <see cref="IMasterKeyProvider.KekId"/> that identifies <i>those</i> bytes. Obtained from
+/// <see cref="IMasterKeyProvider.GetSnapshot"/> once per cipher operation, so the id stamped on a
+/// row and the key that wraps it can never come from two different reads of a source that changed
+/// in between (a key file replaced mid-operation).
+/// </summary>
+/// <remarks>
+/// The snapshot owns its key buffer: <see cref="Dispose"/> zeroes it with
+/// <see cref="CryptographicOperations.ZeroMemory"/>. Always dispose a snapshot
+/// (<c>using var snapshot = provider.GetSnapshot();</c>) and never retain <see cref="Key"/> past it.
+/// </remarks>
+public sealed class MasterKeySnapshot : IDisposable
+{
+    private readonly byte[] _key;
+
+    /// <summary>
+    /// Creates a snapshot that takes ownership of <paramref name="key"/> (it is zeroed on
+    /// <see cref="Dispose"/>; pass a buffer nothing else uses).
+    /// </summary>
+    /// <param name="kekId">The non-secret id of <paramref name="key"/>.</param>
+    /// <param name="key">The key bytes; ownership transfers to the snapshot.</param>
+    public MasterKeySnapshot(string kekId, byte[] key)
+    {
+        KekId = kekId ?? throw new ArgumentNullException(nameof(kekId));
+        _key = key ?? throw new ArgumentNullException(nameof(key));
+    }
+
+    /// <summary>The non-secret identifier of <see cref="Key"/>.</summary>
+    public string KekId { get; }
+
+    /// <summary>The key bytes. Valid only until <see cref="Dispose"/>, which zeroes them.</summary>
+    public ReadOnlyMemory<byte> Key => _key;
+
+    /// <summary>Zeroes the key buffer.</summary>
+    public void Dispose() => CryptographicOperations.ZeroMemory(_key);
+}
@@ -22,8 +22,15 @@ namespace ZB.MOM.WW.Secrets.Crypto;
 /// constant-time comparison is required. Every cryptographic failure path — a mismatched tag,
 /// a wrong or unknown KEK, or any other <see cref="CryptographicException"/> — is surfaced as a
 /// <see cref="SecretDecryptionException"/>; the cipher never returns a wrong plaintext and never
-/// leaks a raw cryptographic exception. The transient DEK buffer is always zeroed with
-/// <see cref="CryptographicOperations.ZeroMemory"/>.
+/// leaks a raw cryptographic exception. A malformed row (a wrong-length nonce, tag or wrapped DEK,
+/// which <see cref="AesGcm"/> reports as <see cref="ArgumentException"/>) fails the same way. The
+/// transient DEK buffer is always zeroed with <see cref="CryptographicOperations.ZeroMemory"/>.
+/// </para>
+/// <para>
+/// Every operation takes exactly one <see cref="MasterKeySnapshot"/> per KEK
+/// (<see cref="IMasterKeyProvider.GetSnapshot"/>), so the KEK id stamped on or checked against a row
+/// and the key used for it always come from the same reading, and the snapshot's key copy is zeroed
+/// when the operation ends.
 /// </para>
 /// </remarks>
 public sealed class AesGcmEnvelopeCipher : ISecretCipher
@@ -49,8 +56,9 @@ public sealed class AesGcmEnvelopeCipher : ISecretCipher
    {
        ArgumentNullException.ThrowIfNull(plaintext);

-        string kekId = _masterKeyProvider.KekId;
-        ReadOnlySpan<byte> masterKey = _masterKeyProvider.GetMasterKey().Span;
+        using MasterKeySnapshot kek = _masterKeyProvider.GetSnapshot();
+        string kekId = kek.KekId;
+        ReadOnlySpan<byte> masterKey = kek.Key.Span;

        byte[] dek = new byte[KeySizeBytes];
        try
@@ -110,13 +118,13 @@ public sealed class AesGcmEnvelopeCipher : ISecretCipher
    {
        ArgumentNullException.ThrowIfNull(secret);

-        string kekId = _masterKeyProvider.KekId;
-        if (!string.Equals(kekId, secret.KekId, StringComparison.Ordinal))
+        using MasterKeySnapshot kek = _masterKeyProvider.GetSnapshot();
+        if (!string.Equals(kek.KekId, secret.KekId, StringComparison.Ordinal))
        {
            throw new SecretDecryptionException($"Row wrapped by unknown KEK '{secret.KekId}'.");
        }

-        ReadOnlySpan<byte> masterKey = _masterKeyProvider.GetMasterKey().Span;
+        ReadOnlySpan<byte> masterKey = kek.Key.Span;

        byte[] dek = new byte[KeySizeBytes];
        try
@@ -148,6 +156,15 @@ public sealed class AesGcmEnvelopeCipher : ISecretCipher
            throw new SecretDecryptionException(
                $"Failed to decrypt secret '{secret.Name.Value}'.", ex);
        }
+        // AesGcm throws ArgumentException for a wrong-length nonce or tag, or a ciphertext/plaintext
+        // length mismatch (a WrappedDek that is not 32 bytes): a corrupt or truncated row. It is a
+        // decryption failure like any other, so callers (the resolver's decryption-failed audit)
+        // classify it correctly (Secrets-004). Mirrors Rewrap.
+        catch (ArgumentException ex)
+        {
+            throw new SecretDecryptionException(
+                $"Failed to decrypt secret '{secret.Name.Value}': the row is malformed.", ex);
+        }
        finally
        {
            CryptographicOperations.ZeroMemory(dek);
@@ -161,18 +178,21 @@ public sealed class AesGcmEnvelopeCipher : ISecretCipher
        ArgumentNullException.ThrowIfNull(oldKek);
        ArgumentNullException.ThrowIfNull(newKek);

+        using MasterKeySnapshot oldSnapshot = oldKek.GetSnapshot();
+
        // Fail closed if the row is not actually wrapped by the supplied old KEK — a caller must
        // not silently "rewrap" a row that is already on a different key.
-        if (!string.Equals(secret.KekId, oldKek.KekId, StringComparison.Ordinal))
+        if (!string.Equals(secret.KekId, oldSnapshot.KekId, StringComparison.Ordinal))
        {
            throw new SecretDecryptionException(
                $"Cannot rewrap secret '{secret.Name.Value}': row is wrapped by KEK '{secret.KekId}', " +
-                $"not the supplied old KEK '{oldKek.KekId}'.");
+                $"not the supplied old KEK '{oldSnapshot.KekId}'.");
        }

-        ReadOnlySpan<byte> oldMasterKey = oldKek.GetMasterKey().Span;
-        ReadOnlySpan<byte> newMasterKey = newKek.GetMasterKey().Span;
-        string newKekId = newKek.KekId;
+        using MasterKeySnapshot newSnapshot = newKek.GetSnapshot();
+        ReadOnlySpan<byte> oldMasterKey = oldSnapshot.Key.Span;
+        ReadOnlySpan<byte> newMasterKey = newSnapshot.Key.Span;
+        string newKekId = newSnapshot.KekId;

        byte[] dek = new byte[KeySizeBytes];
        try
@@ -1,3 +1,4 @@
+using System.Security.Cryptography;
 using System.Text;
 using ZB.MOM.WW.Secrets.Abstractions;

@@ -45,6 +46,7 @@ public sealed class FileMasterKeyProvider : MasterKeyProviderBase

        // Otherwise treat the content as base64 text.
        string text = Encoding.UTF8.GetString(raw).Trim();
+        CryptographicOperations.ZeroMemory(raw);
        try
        {
            return Convert.FromBase64String(text);
@@ -13,6 +13,11 @@ namespace ZB.MOM.WW.Secrets.MasterKey;
 /// file value takes effect without recreating the provider. <see cref="KekId"/> is likewise derived
 /// from the currently resolved key (unless an explicit id is configured), so it stays stable for a
 /// given key and shifts only when the key itself changes.
+/// <para>
+/// <see cref="GetSnapshot"/> resolves the source <b>once</b> and derives the id from that same
+/// reading, so a key file replaced mid-operation can never produce a row stamped with one key's id
+/// and wrapped under another (Secrets-007). Intermediate key buffers are zeroed.
+/// </para>
 /// </remarks>
 public abstract class MasterKeyProviderBase : IMasterKeyProvider
 {
@@ -45,7 +50,32 @@ public abstract class MasterKeyProviderBase : IMasterKeyProvider
                return explicitId;
            }

-            return DeriveKekId(ResolveValidatedKey());
+            byte[] key = ResolveValidatedKey();
+            try
+            {
+                return DeriveKekId(key);
+            }
+            finally
+            {
+                CryptographicOperations.ZeroMemory(key);
+            }
+        }
+    }
+
+    /// <inheritdoc />
+    public MasterKeySnapshot GetSnapshot()
+    {
+        byte[] key = ResolveValidatedKey();
+        try
+        {
+            string? explicitId = _options.KekId;
+            string kekId = string.IsNullOrEmpty(explicitId) ? DeriveKekId(key) : explicitId;
+            return new MasterKeySnapshot(kekId, key);
+        }
+        catch
+        {
+            CryptographicOperations.ZeroMemory(key);
+            throw;
        }
    }

@@ -54,7 +84,10 @@ public abstract class MasterKeyProviderBase : IMasterKeyProvider
    /// <see cref="MasterKeyUnavailableException"/> when the source is missing or malformed; length
    /// validation is applied by the base class.
    /// </summary>
-    /// <returns>The resolved key bytes (any length; validated by the caller).</returns>
+    /// <returns>
+    /// The resolved key bytes (any length; validated by the caller), in a <b>freshly allocated</b>
+    /// buffer the caller owns: the base class zeroes it after use, so never return a cached array.
+    /// </returns>
    protected abstract byte[] ResolveKeyBytes();

    private byte[] ResolveValidatedKey()
@@ -62,6 +95,7 @@ public abstract class MasterKeyProviderBase : IMasterKeyProvider
        byte[] key = ResolveKeyBytes();
        if (key.Length != KeySizeBytes)
        {
+            CryptographicOperations.ZeroMemory(key);
            throw new MasterKeyUnavailableException(
                $"Master key must be exactly {KeySizeBytes} bytes (AES-256) but was {key.Length} bytes.");
        }
@@ -0,0 +1,64 @@
+using ZB.MOM.WW.Audit;
+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.Crypto;
+
+/// <summary>
+/// Secrets-004: a row whose crypto fields have the wrong length (truncated by a buggy replication
+/// peer or a hand-edited database) must fail as <see cref="SecretDecryptionException"/> — the only
+/// failure <see cref="ISecretCipher.Decrypt"/> documents — never as a raw <see cref="ArgumentException"/>.
+/// </summary>
+public class MalformedRowTests
+{
+    public static TheoryData<string> Fields => new() { "Nonce", "Tag", "WrappedDek", "WrapNonce", "WrapTag" };
+
+    private static StoredSecret Truncate(StoredSecret row, string field) => field switch
+    {
+        "Nonce" => row with { Nonce = new byte[5] },
+        "Tag" => row with { Tag = new byte[3] },
+        "WrappedDek" => row with { WrappedDek = row.WrappedDek[..31] },
+        "WrapNonce" => row with { WrapNonce = new byte[5] },
+        "WrapTag" => row with { WrapTag = new byte[3] },
+        _ => throw new ArgumentOutOfRangeException(nameof(field)),
+    };
+
+    [Theory]
+    [MemberData(nameof(Fields))]
+    public void A_wrong_length_field_fails_as_a_decryption_error(string field)
+    {
+        var cipher = new AesGcmEnvelopeCipher(new FakeMasterKeyProvider("k1"));
+        StoredSecret row = cipher.Encrypt(new SecretName("sql/foo"), "hunter2", SecretContentType.Text);
+        Assert.Equal("hunter2", cipher.Decrypt(row)); // positive control: the untouched row decrypts
+
+        StoredSecret corrupt = Truncate(row, field);
+
+        var ex = Assert.Throws<SecretDecryptionException>(() => cipher.Decrypt(corrupt));
+        Assert.IsAssignableFrom<ArgumentException>(ex.InnerException);
+        Assert.DoesNotContain("hunter2", ex.Message);
+    }
+
+    [Fact]
+    public async Task The_resolver_audits_a_malformed_row_as_decryption_failed()
+    {
+        var cipher = new AesGcmEnvelopeCipher(new FakeMasterKeyProvider("k1"));
+        var store = new CountingSecretStore();
+        var audit = new CapturingAuditWriter();
+        var resolver = new DefaultSecretResolver(store, cipher, audit, TimeSpan.FromSeconds(30));
+        var name = new SecretName("sql/foo");
+        StoredSecret row = cipher.Encrypt(name, "hunter2", SecretContentType.Text) with
+        {
+            Revision = 1,
+            CreatedUtc = DateTimeOffset.UnixEpoch,
+            UpdatedUtc = DateTimeOffset.UnixEpoch,
+        };
+        store.Seed(row with { WrappedDek = row.WrappedDek[..31] });
+
+        await Assert.ThrowsAsync<SecretDecryptionException>(() => resolver.GetAsync(name, CancellationToken.None));
+
+        AuditEvent evt = Assert.Single(audit.Events);
+        Assert.Equal("secret.resolve.decryption-failed", evt.Action);
+        Assert.Equal(AuditOutcome.Failure, evt.Outcome);
+    }
+}
@@ -0,0 +1,170 @@
+using System.Security.Cryptography;
+using ZB.MOM.WW.Secrets.Abstractions;
+using ZB.MOM.WW.Secrets.Crypto;
+using ZB.MOM.WW.Secrets.MasterKey;
+using ZB.MOM.WW.Secrets.Tests.Fakes;
+
+namespace ZB.MOM.WW.Secrets.Tests.Crypto;
+
+/// <summary>
+/// Secrets-007: the cipher must read the KEK id and the key from ONE resolution of the source, and
+/// zero the key copy it used. Also pins the on-disk format: a row written by the pre-snapshot cipher
+/// (0.6.5) must still decrypt.
+/// </summary>
+public class MasterKeySnapshotTests
+{
+    private static readonly byte[] KeyA = Enumerable.Range(0, 32).Select(i => (byte)i).ToArray();
+    private static readonly byte[] KeyB = Enumerable.Range(100, 32).Select(i => (byte)i).ToArray();
+
+    /// <summary>
+    /// A file-like provider whose source is replaced after its first read: the first resolution
+    /// returns key A, every later one key B (an operator swapping the key file mid-operation).
+    /// </summary>
+    private sealed class SwappedOnceProvider : MasterKeyProviderBase
+    {
+        private int _reads;
+
+        public SwappedOnceProvider() : base(new MasterKeyOptions()) { }
+
+        public int Reads => _reads;
+
+        protected override byte[] ResolveKeyBytes() =>
+            (byte[])(Interlocked.Increment(ref _reads) == 1 ? KeyA : KeyB).Clone();
+    }
+
+    private sealed class FixedKeyProvider : MasterKeyProviderBase
+    {
+        private readonly byte[] _key;
+
+        public FixedKeyProvider(byte[] key) : base(new MasterKeyOptions()) => _key = key;
+
+        protected override byte[] ResolveKeyBytes() => (byte[])_key.Clone();
+    }
+
+    /// <summary>Records the key buffer of every snapshot it hands out.</summary>
+    private sealed class RecordingProvider(byte[] key, string kekId) : IMasterKeyProvider
+    {
+        public List<byte[]> Handed { get; } = new();
+
+        public string KekId => kekId;
+
+        public ReadOnlyMemory<byte> GetMasterKey() => (byte[])key.Clone();
+
+        public MasterKeySnapshot GetSnapshot()
+        {
+            byte[] copy = (byte[])key.Clone();
+            Handed.Add(copy);
+            return new MasterKeySnapshot(kekId, copy);
+        }
+    }
+
+    [Fact]
+    public void Encrypt_stamps_the_id_of_the_key_that_actually_wrapped_the_row()
+    {
+        var swapping = new SwappedOnceProvider();
+        var cipher = new AesGcmEnvelopeCipher(swapping);
+
+        StoredSecret row = cipher.Encrypt(new SecretName("sql/foo"), "hunter2", SecretContentType.Text);
+
+        // One resolution per operation, not one for the id and another for the key.
+        Assert.Equal(1, swapping.Reads);
+
+        // The row claims key A and is wrapped by key A, so a node holding key A reads it.
+        var holderOfA = new AesGcmEnvelopeCipher(new FixedKeyProvider(KeyA));
+        Assert.Equal(new FixedKeyProvider(KeyA).KekId, row.KekId);
+        Assert.Equal("hunter2", holderOfA.Decrypt(row));
+    }
+
+    [Fact]
+    public void Decrypt_and_rewrap_resolve_each_kek_once()
+    {
+        var aCipher = new AesGcmEnvelopeCipher(new FixedKeyProvider(KeyA));
+        StoredSecret row = aCipher.Encrypt(new SecretName("sql/foo"), "hunter2", SecretContentType.Text);
+
+        var swapping = new SwappedOnceProvider();
+        Assert.Equal("hunter2", new AesGcmEnvelopeCipher(swapping).Decrypt(row));
+        Assert.Equal(1, swapping.Reads);
+
+        var oldKek = new SwappedOnceProvider();
+        var newKek = new FixedKeyProvider(KeyB);
+        StoredSecret rewrapped = aCipher.Rewrap(row, oldKek, newKek);
+        Assert.Equal(1, oldKek.Reads);
+        Assert.Equal("hunter2", new AesGcmEnvelopeCipher(new FixedKeyProvider(KeyB)).Decrypt(rewrapped));
+    }
+
+    [Fact]
+    public void The_cipher_zeroes_every_key_snapshot_it_used()
+    {
+        var provider = new RecordingProvider(KeyA, "k1");
+        var cipher = new AesGcmEnvelopeCipher(provider);
+
+        StoredSecret row = cipher.Encrypt(new SecretName("sql/foo"), "hunter2", SecretContentType.Text);
+        Assert.Equal("hunter2", cipher.Decrypt(row));
+        cipher.Rewrap(row, provider, new RecordingProvider(KeyB, "k2"));
+
+        Assert.Equal(3, provider.Handed.Count);
+        Assert.All(provider.Handed, buffer => Assert.All(buffer, b => Assert.Equal(0, b)));
+    }
+
+    [Fact]
+    public void The_default_snapshot_copies_so_disposing_it_never_zeroes_the_providers_own_key()
+    {
+        byte[] key = (byte[])KeyA.Clone();
+        var provider = new FakeMasterKeyProvider("k1", key);
+        var cipher = new AesGcmEnvelopeCipher(provider);
+
+        StoredSecret row = cipher.Encrypt(new SecretName("sql/foo"), "hunter2", SecretContentType.Text);
+
+        // The provider (which hands out its cached array) still works after the cipher zeroed its snapshot.
+        Assert.Equal("hunter2", cipher.Decrypt(row));
+        Assert.Equal(KeyA, provider.GetMasterKey().ToArray());
+    }
+
+    [Fact]
+    public void A_row_written_by_the_0_6_5_cipher_still_decrypts()
+    {
+        // Generated with the 0.6.5 AesGcmEnvelopeCipher + FileMasterKeyProvider over a raw key file
+        // holding bytes 0..31 (derived KEK id). The format is unchanged by the snapshot work.
+        string keyPath = Path.Combine(Path.GetTempPath(), $"zb-secrets-golden-{Guid.NewGuid():N}.key");
+        File.WriteAllBytes(keyPath, KeyA);
+        try
+        {
+            var provider = new FileMasterKeyProvider(new MasterKeyOptions
+            {
+                Source = MasterKeySource.File,
+                FilePath = keyPath,
+            });
+            var row = new StoredSecret
+            {
+                Name = new SecretName("sql/golden"),
+                ContentType = SecretContentType.Text,
+                Ciphertext = Convert.FromHexString("65D1B5D4FD8F538D9B58A622E202D84CDACD1F17D809DEBA0E1575DB"),
+                Nonce = Convert.FromHexString("023EF785E6D7318EB0F1A238"),
+                Tag = Convert.FromHexString("AAFF7A1E634C74332D4D4B75B36DE293"),
+                WrappedDek = Convert.FromHexString("8BA7B8BD1A289CF97AE91386B72061C97A7CD7FF9487140590015A0ED46E79B2"),
+                WrapNonce = Convert.FromHexString("04B946E3103E0E523714DDA8"),
+                WrapTag = Convert.FromHexString("F542CA1BF350577B400E524B816D64A3"),
+                KekId = "sha256:630dcd2966c4",
+                Revision = 1,
+                CreatedUtc = DateTimeOffset.UnixEpoch,
+                UpdatedUtc = DateTimeOffset.UnixEpoch,
+            };
+
+            Assert.Equal("sha256:630dcd2966c4", provider.KekId);
+            Assert.Equal("correct horse battery staple", new AesGcmEnvelopeCipher(provider).Decrypt(row));
+        }
+        finally
+        {
+            File.Delete(keyPath);
+        }
+    }
+
+    [Fact]
+    public void Disposing_a_snapshot_zeroes_its_key()
+    {
+        byte[] key = RandomNumberGenerator.GetBytes(32);
+        var snapshot = new MasterKeySnapshot("k1", key);
+        snapshot.Dispose();
+        Assert.All(key, b => Assert.Equal(0, b));
+    }
+}