Public Access
Pin every cipher's AAD resource type from one table, not one test each
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.
This commit is contained in:
@@ -53,22 +53,55 @@ public sealed class AadResourceTypeTests
|
|||||||
+ "Either the enums were renumbered or this pairing needs re-checking by hand.");
|
+ "Either the enums were renumbered or this pairing needs re-checking by hand.");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The specified pairing of wire type to AAD resource type, stated out of band, one row per cipher.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// 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
|
||||||
|
/// <see cref="EverySynchronisedType_HasItsCipherPinnedHere"/>.
|
||||||
|
/// </remarks>
|
||||||
|
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<SyncEntityType, CryptoSpec.AadResourceType> Pinned
|
||||||
|
{
|
||||||
|
get
|
||||||
|
{
|
||||||
|
var data = new TheoryData<SyncEntityType, CryptoSpec.AadResourceType>();
|
||||||
|
|
||||||
|
foreach (var (wire, resource) in PinnedPairs)
|
||||||
|
{
|
||||||
|
data.Add(wire, resource);
|
||||||
|
}
|
||||||
|
|
||||||
|
return data;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// Opened <em>independently</em>, through the low-level <c>ItemKeys</c> API with the resource type this
|
/// Opened <em>independently</em>, through the low-level <c>ItemKeys</c> 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
|
/// table names rather than the one the cipher holds. That is the whole point, and it is the property two
|
||||||
/// instructive way: it checked only that a key payload does not open as a host and vice versa, which is
|
/// earlier versions of this file lacked: checking that a key payload does not open as a host is true
|
||||||
/// true however both ciphers are misconfigured. <c>Seal</c> and <c>TryOpen</c> share one constant, so
|
/// however both ciphers are misconfigured, because <c>Seal</c> and <c>TryOpen</c> share one constant. A
|
||||||
/// changing it changes both, the round trip still works, and the two ciphers still differ from each
|
/// test that compares an implementation against itself cannot catch a self-consistent mistake.
|
||||||
/// other. Sealing every private key as if it were a vault passed all of it.
|
|
||||||
/// </para>
|
/// </para>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// A test that only compares an implementation against itself cannot catch a self-consistent mistake.
|
/// Written as a table over every cipher, not one test per cipher, because the same hole was found three
|
||||||
/// This one states the specified value out of band and refuses anything else.
|
/// times — twice by mutation testing after the fact. Pointing <c>CredentialCipher</c> at
|
||||||
|
/// <c>AadResourceType.Vault</c> passed the entire suite until this existed.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
[Fact]
|
[Theory]
|
||||||
public void AKeyPayload_OpensUnderTheResourceTypeTheSpecificationNames()
|
[MemberData(nameof(Pinned))]
|
||||||
|
public void EveryCipher_SealsUnderTheResourceTypeTheSpecificationNames(
|
||||||
|
SyncEntityType wire,
|
||||||
|
CryptoSpec.AadResourceType resource)
|
||||||
{
|
{
|
||||||
var vaultKey = RandomNumberGenerator.GetBytes(32);
|
var vaultKey = RandomNumberGenerator.GetBytes(32);
|
||||||
|
|
||||||
@@ -76,51 +109,91 @@ public sealed class AadResourceTypeTests
|
|||||||
const uint Generation = 1;
|
const uint Generation = 1;
|
||||||
const uint Version = 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(
|
var dataKey = ItemKeys.TryUnwrapDataKey(
|
||||||
vaultKey,
|
vaultKey, payload.WrappedDataKey, resource, entityId, Generation, Version);
|
||||||
payload.WrappedDataKey,
|
|
||||||
CryptoSpec.AadResourceType.SshKey,
|
|
||||||
entityId,
|
|
||||||
Generation,
|
|
||||||
Version);
|
|
||||||
|
|
||||||
dataKey.ShouldNotBeNull(
|
dataKey.ShouldNotBeNull(
|
||||||
"SshKeyCipher must wrap the data key under AadResourceType.SshKey; if this is null it used "
|
$"The {wire} cipher must wrap its data key under AadResourceType.{resource}; a null here means "
|
||||||
+ "some other resource type, which round-trips fine and violates docs/crypto.md.");
|
+ "it used some other resource type, which round-trips fine and violates docs/crypto.md.");
|
||||||
|
|
||||||
ItemKeys.TryOpenPayload(
|
ItemKeys.TryOpenPayload(
|
||||||
dataKey,
|
dataKey,
|
||||||
payload.Envelope,
|
payload.Envelope,
|
||||||
CryptoSpec.AadResourceType.SshKey,
|
resource,
|
||||||
entityId,
|
entityId,
|
||||||
payload.DataKeyId,
|
payload.DataKeyId,
|
||||||
Generation,
|
Generation,
|
||||||
Version).ShouldNotBeNull("and it must seal the envelope under the same resource type.");
|
Version).ShouldNotBeNull("and it must seal the envelope under the same resource type.");
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <remarks>Pins the host cipher the same way, since the two are now easy to confuse for each other.</remarks>
|
/// <remarks>
|
||||||
|
/// 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.
|
||||||
|
/// </remarks>
|
||||||
[Fact]
|
[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 vaultKey = RandomNumberGenerator.GetBytes(32);
|
||||||
|
|
||||||
var entityId = Guid.CreateVersion7();
|
var entityId = Guid.CreateVersion7();
|
||||||
const uint Generation = 1;
|
|
||||||
const uint Version = 1;
|
|
||||||
|
|
||||||
var host = new HostSecret { Label = "prod-db", Hostname = "db.internal" };
|
var sealed_ = CredentialCipher.Seal(
|
||||||
var payload = HostCipher.Seal(host, vaultKey, entityId, Generation, (int)Version);
|
NewCredential(), vaultKey, entityId, keyGeneration: 1, itemVersion: 1);
|
||||||
|
|
||||||
ItemKeys.TryUnwrapDataKey(
|
HostCipher.TryOpen(sealed_, vaultKey, entityId, itemVersion: 1).ShouldBeNull();
|
||||||
vaultKey,
|
SshKeyCipher.TryOpen(sealed_, vaultKey, entityId, itemVersion: 1).ShouldBeNull();
|
||||||
payload.WrappedDataKey,
|
CredentialCipher.TryOpen(sealed_, vaultKey, entityId, itemVersion: 1).ShouldNotBeNull();
|
||||||
CryptoSpec.AadResourceType.Host,
|
}
|
||||||
entityId,
|
|
||||||
Generation,
|
[Fact]
|
||||||
Version)
|
public void ACredentialSealedAtOneVersion_DoesNotOpenAtAnother()
|
||||||
.ShouldNotBeNull("HostCipher must wrap the data key under AadResourceType.Host.");
|
{
|
||||||
|
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]
|
[Fact]
|
||||||
@@ -183,6 +256,14 @@ public sealed class AadResourceTypeTests
|
|||||||
SshKeyCipher.TryOpen(payload, vaultKey, entityId, itemVersion: 3).ShouldBeNull();
|
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()
|
private static SshKeySecret NewKey() => new()
|
||||||
{
|
{
|
||||||
Label = "deploy",
|
Label = "deploy",
|
||||||
|
|||||||
Reference in New Issue
Block a user