From d10a38d8e61a956c2d63385b8f8295035b26a422 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Wed, 29 Jul 2026 21:17:36 +0200 Subject: [PATCH] Pin every cipher's AAD resource type from one table, not one test each MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutation testing found that pointing CredentialCipher at AadResourceType.Vault passed the entire suite. Every credential test compared the cipher against itself — round trips, cross-type refusals, two-machine sync — and all of those stay true when both halves of one cipher are wrong together, because Seal and TryOpen share the constant. A password sealed under the resource type for a vault encrypts cleanly, decrypts cleanly, syncs cleanly, and violates docs/crypto.md in a way nothing surfaces until another implementation refuses the item. By then the AAD is frozen into stored ciphertext and only clients can re-encrypt it. This is the third time that hole has appeared in this file, and the second time mutation testing rather than review is what found it. So the fix is structural rather than another hand-written test: one table of wire type to resource type, a theory that seals a sample through each cipher and opens it with the resource type the table names — never the one the cipher holds — and a guard asserting the table covers ItemKinds.SyncedTypes. A fourth item type can no longer be added without pinning its resource type: the coverage test fails, and the sample switch throws with an explanation. The two per-cipher tests it replaces said the same thing for hosts and keys, so nothing is lost and the credential row is no longer something someone has to remember. Verified by re-running the mutation matrix. All seven sabotages are now detected: the credential merge dropping its redaction, the key/credential exclusivity check disabled, the schema version ladder flattened so a key-bound host claims the credential version, a credential sending the server an empty fields record instead of none, CredentialKind claiming to be a host, CredentialCipher sealing under the wrong resource type, and the credential noun reading "host". Two of those were unproven before this run — one because the earlier sabotage did not compile, and one because it was genuinely undetected. Sync.Tests 88/88, Domain.Tests 117/117. Zero warnings, dotnet format clean. --- .../AadResourceTypeTests.cs | 147 ++++++++++++++---- 1 file changed, 114 insertions(+), 33 deletions(-) diff --git a/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs b/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs index 6f4b69a..2af46df 100644 --- a/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs +++ b/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs @@ -53,22 +53,55 @@ public sealed class AadResourceTypeTests + "Either the enums were renumbered or this pairing needs re-checking by hand."); } + /// + /// The specified pairing of wire type to AAD resource type, stated out of band, one row per cipher. + /// + /// + /// The single source for both tests below: what each cipher must use, and which types must have a cipher + /// pinned at all. Adding an item type without adding a row here fails + /// . + /// + private static readonly (SyncEntityType Wire, CryptoSpec.AadResourceType Resource)[] PinnedPairs = + [ + (SyncEntityType.Host, CryptoSpec.AadResourceType.Host), + (SyncEntityType.SshKey, CryptoSpec.AadResourceType.SshKey), + (SyncEntityType.Credential, CryptoSpec.AadResourceType.Credential), + ]; + + public static TheoryData Pinned + { + get + { + var data = new TheoryData(); + + foreach (var (wire, resource) in PinnedPairs) + { + data.Add(wire, resource); + } + + return data; + } + } + /// /// /// Opened independently, through the low-level ItemKeys API with the resource type this - /// test names itself. That is the whole point, and the first version of this file got it wrong in an - /// instructive way: it checked only that a key payload does not open as a host and vice versa, which is - /// true however both ciphers are misconfigured. Seal and TryOpen share one constant, so - /// changing it changes both, the round trip still works, and the two ciphers still differ from each - /// other. Sealing every private key as if it were a vault passed all of it. + /// table names rather than the one the cipher holds. That is the whole point, and it is the property two + /// earlier versions of this file lacked: checking that a key payload does not open as a host is true + /// however both ciphers are misconfigured, because Seal and TryOpen share one constant. A + /// test that compares an implementation against itself cannot catch a self-consistent mistake. /// /// - /// A test that only compares an implementation against itself cannot catch a self-consistent mistake. - /// This one states the specified value out of band and refuses anything else. + /// Written as a table over every cipher, not one test per cipher, because the same hole was found three + /// times — twice by mutation testing after the fact. Pointing CredentialCipher at + /// AadResourceType.Vault passed the entire suite until this existed. /// /// - [Fact] - public void AKeyPayload_OpensUnderTheResourceTypeTheSpecificationNames() + [Theory] + [MemberData(nameof(Pinned))] + public void EveryCipher_SealsUnderTheResourceTypeTheSpecificationNames( + SyncEntityType wire, + CryptoSpec.AadResourceType resource) { var vaultKey = RandomNumberGenerator.GetBytes(32); @@ -76,51 +109,91 @@ public sealed class AadResourceTypeTests const uint Generation = 1; const uint Version = 1; - var payload = SshKeyCipher.Seal(NewKey(), vaultKey, entityId, Generation, (int)Version); + var payload = SealSample(wire, vaultKey, entityId, Generation, (int)Version); var dataKey = ItemKeys.TryUnwrapDataKey( - vaultKey, - payload.WrappedDataKey, - CryptoSpec.AadResourceType.SshKey, - entityId, - Generation, - Version); + vaultKey, payload.WrappedDataKey, resource, entityId, Generation, Version); dataKey.ShouldNotBeNull( - "SshKeyCipher must wrap the data key under AadResourceType.SshKey; if this is null it used " - + "some other resource type, which round-trips fine and violates docs/crypto.md."); + $"The {wire} cipher must wrap its data key under AadResourceType.{resource}; a null here means " + + "it used some other resource type, which round-trips fine and violates docs/crypto.md."); ItemKeys.TryOpenPayload( dataKey, payload.Envelope, - CryptoSpec.AadResourceType.SshKey, + resource, entityId, payload.DataKeyId, Generation, Version).ShouldNotBeNull("and it must seal the envelope under the same resource type."); } - /// Pins the host cipher the same way, since the two are now easy to confuse for each other. + /// + /// The guard that makes the table above self-maintaining. A fourth item type would otherwise sync, + /// encrypt and merge correctly while being sealed under any resource type at all, and nothing would say + /// so until another implementation refused the item — by which point the AAD is frozen into stored + /// ciphertext and only clients can re-encrypt it. + /// [Fact] - public void AHostPayload_OpensUnderTheResourceTypeTheSpecificationNames() + public void EverySynchronisedType_HasItsCipherPinnedHere() + { + PinnedPairs.Select(pair => pair.Wire) + .ShouldBe(ItemKinds.SyncedTypes, ignoreOrder: true); + } + + private static EncryptedPayload SealSample( + SyncEntityType wire, + byte[] vaultKey, + Guid entityId, + uint generation, + int version) => wire switch + { + SyncEntityType.Host => HostCipher.Seal( + new HostSecret { Label = "prod-db", Hostname = "db.internal" }, + vaultKey, + entityId, + generation, + version), + + SyncEntityType.SshKey => SshKeyCipher.Seal( + NewKey(), vaultKey, entityId, generation, version), + + SyncEntityType.Credential => CredentialCipher.Seal( + NewCredential(), vaultKey, entityId, generation, version), + + _ => throw new ArgumentOutOfRangeException( + nameof(wire), + wire, + "No sample exists for this item type. Add one when adding the type, or the pairing above " + + "cannot be checked."), + }; + + [Fact] + public void ACredentialPayload_OpensAsNothingElse() { var vaultKey = RandomNumberGenerator.GetBytes(32); var entityId = Guid.CreateVersion7(); - const uint Generation = 1; - const uint Version = 1; - var host = new HostSecret { Label = "prod-db", Hostname = "db.internal" }; - var payload = HostCipher.Seal(host, vaultKey, entityId, Generation, (int)Version); + var sealed_ = CredentialCipher.Seal( + NewCredential(), vaultKey, entityId, keyGeneration: 1, itemVersion: 1); - ItemKeys.TryUnwrapDataKey( - vaultKey, - payload.WrappedDataKey, - CryptoSpec.AadResourceType.Host, - entityId, - Generation, - Version) - .ShouldNotBeNull("HostCipher must wrap the data key under AadResourceType.Host."); + HostCipher.TryOpen(sealed_, vaultKey, entityId, itemVersion: 1).ShouldBeNull(); + SshKeyCipher.TryOpen(sealed_, vaultKey, entityId, itemVersion: 1).ShouldBeNull(); + CredentialCipher.TryOpen(sealed_, vaultKey, entityId, itemVersion: 1).ShouldNotBeNull(); + } + + [Fact] + public void ACredentialSealedAtOneVersion_DoesNotOpenAtAnother() + { + var vaultKey = RandomNumberGenerator.GetBytes(32); + + var entityId = Guid.CreateVersion7(); + + var payload = CredentialCipher.Seal( + NewCredential(), vaultKey, entityId, keyGeneration: 1, itemVersion: 2); + + CredentialCipher.TryOpen(payload, vaultKey, entityId, itemVersion: 3).ShouldBeNull(); } [Fact] @@ -183,6 +256,14 @@ public sealed class AadResourceTypeTests SshKeyCipher.TryOpen(payload, vaultKey, entityId, itemVersion: 3).ShouldBeNull(); } + private static CredentialSecret NewCredential() => new() + { + Label = "db-login", + Password = "hunter2", + Username = "postgres", + Notes = "used by CI", + }; + private static SshKeySecret NewKey() => new() { Label = "deploy",