Public Access
Add the client's SSH key model, codec, merge and cipher
The client can now seal and open an SSH key item. Nothing consumes it yet — the repository, the sync engine's per-type handling and the UI come next — but this is the layer everything above it depends on, and it is the layer where the crypto has to be right. SshKeySecret holds the private key as an ordinary string, deliberately, and says so: a .NET string cannot be wiped, so the material lives until the GC reuses the memory. libsodium's guarded memory was considered and rejected because the passphrase protecting the key, the password on the next item and the JSON the codec just parsed are all strings on the same heap — protecting one field among them reads as security and buys nothing. What the design does give is that the key never reaches the disk in plaintext, never reaches the server at all, and is handed to SSH.NET through a MemoryStream so there is no temporary key file to leak. Validation refuses a public key by name. ssh-keygen writes two files whose names differ by four characters, and pasting the wrong one otherwise produces a vault item that looks fine and fails at connection time with an authentication error that says nothing about which file you chose. The merge redacts the private key and its passphrase from the conflict log. A host conflict shows both values so the loser can be put back; doing that for a private key would write the discarded key into a log that is designed to be read rather than used and is deliberately retained after acknowledgement. Two different private keys are not something anyone reconciles by reading them side by side. And the lesson worth recording, because it nearly shipped: the first version of AadResourceTypeTests proved nothing. It checked that a key payload does not open as a host and vice versa — true however both ciphers are misconfigured, because Seal and TryOpen share one constant, so changing it changes both and the round trip still works. Sealing every private key as if it were a vault passed all twelve tests. The tests now open a sealed payload independently through ItemKeys with the resource type named out of band, and that does fail under the same sabotage. A test that only compares an implementation against itself cannot catch a self-consistent mistake. The trap it defends: SyncEntityType.SshKey is 3, AadResourceType.SshKey is 6, because the crypto enum also carries None, User, Device and Vault ahead of the item types. A cast between them is a specification violation that encrypts cleanly and would only surface when another implementation refused the item.
This commit is contained in:
@@ -0,0 +1,194 @@
|
||||
using System.Security.Cryptography;
|
||||
using DodoSSH.Client.Domain;
|
||||
using DodoSSH.Client.Sync;
|
||||
using DodoSSH.Contracts;
|
||||
using DodoSSH.Crypto;
|
||||
|
||||
namespace DodoSSH.Client.Sync.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Each item type must be sealed under its own AAD resource type, and the two enums that name item types
|
||||
/// deliberately do not agree.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <c>SyncEntityType</c> lists only syncable items, so <c>Host</c> is 1 and <c>SshKey</c> is 3.
|
||||
/// <c>CryptoSpec.AadResourceType</c> also covers users, devices and vaults, so the same two are 4 and 6. A
|
||||
/// cipher written by copying its neighbour and casting the wire type would therefore seal a private key as
|
||||
/// if it were a vault — encrypting cleanly, decrypting cleanly on the machine that wrote it, and violating
|
||||
/// docs/crypto.md in a way that surfaces only when another implementation reads the item.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// These tests are cheap and the alternative is a comment. The payload's AAD is frozen, so getting this
|
||||
/// wrong is not something a later release can quietly correct: only clients can re-encrypt, and they can
|
||||
/// only do it if they can still open what is there.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class AadResourceTypeTests
|
||||
{
|
||||
/// <remarks>
|
||||
/// The pairing stated as a table. If <c>AadResourceType</c> is ever renumbered, this is what says so —
|
||||
/// loudly, and before anything is written under the new numbers.
|
||||
/// </remarks>
|
||||
[Theory]
|
||||
[InlineData(SyncEntityType.Host, CryptoSpec.AadResourceType.Host)]
|
||||
[InlineData(SyncEntityType.Credential, CryptoSpec.AadResourceType.Credential)]
|
||||
[InlineData(SyncEntityType.SshKey, CryptoSpec.AadResourceType.SshKey)]
|
||||
[InlineData(SyncEntityType.HostGroup, CryptoSpec.AadResourceType.HostGroup)]
|
||||
[InlineData(SyncEntityType.Tag, CryptoSpec.AadResourceType.Tag)]
|
||||
[InlineData(SyncEntityType.Snippet, CryptoSpec.AadResourceType.Snippet)]
|
||||
[InlineData(SyncEntityType.PortForward, CryptoSpec.AadResourceType.PortForward)]
|
||||
[InlineData(SyncEntityType.KnownHostKey, CryptoSpec.AadResourceType.KnownHostKey)]
|
||||
public void TheTwoEnums_AreNamedAlikeAndNumberedDifferently(
|
||||
SyncEntityType wire,
|
||||
CryptoSpec.AadResourceType resource)
|
||||
{
|
||||
Enum.GetName(wire).ShouldBe(Enum.GetName(resource));
|
||||
|
||||
// The point of the whole file: same name, different number. A test asserting equality here would be
|
||||
// asserting the bug.
|
||||
((int)wire).ShouldNotBe(
|
||||
(int)resource,
|
||||
$"{wire} happens to share a value with its resource type, which makes a cast look correct. "
|
||||
+ "Either the enums were renumbered or this pairing needs re-checking by hand.");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// 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
|
||||
/// 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. <c>Seal</c> and <c>TryOpen</c> 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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public void AKeyPayload_OpensUnderTheResourceTypeTheSpecificationNames()
|
||||
{
|
||||
var vaultKey = RandomNumberGenerator.GetBytes(32);
|
||||
|
||||
var entityId = Guid.CreateVersion7();
|
||||
const uint Generation = 1;
|
||||
const uint Version = 1;
|
||||
|
||||
var payload = SshKeyCipher.Seal(NewKey(), vaultKey, entityId, Generation, (int)Version);
|
||||
|
||||
var dataKey = ItemKeys.TryUnwrapDataKey(
|
||||
vaultKey,
|
||||
payload.WrappedDataKey,
|
||||
CryptoSpec.AadResourceType.SshKey,
|
||||
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.");
|
||||
|
||||
ItemKeys.TryOpenPayload(
|
||||
dataKey,
|
||||
payload.Envelope,
|
||||
CryptoSpec.AadResourceType.SshKey,
|
||||
entityId,
|
||||
payload.DataKeyId,
|
||||
Generation,
|
||||
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>
|
||||
[Fact]
|
||||
public void AHostPayload_OpensUnderTheResourceTypeTheSpecificationNames()
|
||||
{
|
||||
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);
|
||||
|
||||
ItemKeys.TryUnwrapDataKey(
|
||||
vaultKey,
|
||||
payload.WrappedDataKey,
|
||||
CryptoSpec.AadResourceType.Host,
|
||||
entityId,
|
||||
Generation,
|
||||
Version)
|
||||
.ShouldNotBeNull("HostCipher must wrap the data key under AadResourceType.Host.");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AKeyPayload_DoesNotOpenAsAHost()
|
||||
{
|
||||
// Weaker than the two above and kept anyway: it is the property a reader expects to see, and it
|
||||
// covers the case where one cipher is corrected and the other is not.
|
||||
var vaultKey = RandomNumberGenerator.GetBytes(32);
|
||||
|
||||
var entityId = Guid.CreateVersion7();
|
||||
|
||||
var sealedKey = SshKeyCipher.Seal(NewKey(), vaultKey, entityId, keyGeneration: 1, itemVersion: 1);
|
||||
|
||||
HostCipher.TryOpen(sealedKey, vaultKey, entityId, itemVersion: 1).ShouldBeNull();
|
||||
SshKeyCipher.TryOpen(sealedKey, vaultKey, entityId, itemVersion: 1).ShouldNotBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AHostPayload_DoesNotOpenAsAKey()
|
||||
{
|
||||
var vaultKey = RandomNumberGenerator.GetBytes(32);
|
||||
|
||||
var entityId = Guid.CreateVersion7();
|
||||
|
||||
var host = new HostSecret { Label = "prod-db", Hostname = "db.internal" };
|
||||
var sealedHost = HostCipher.Seal(host, vaultKey, entityId, keyGeneration: 1, itemVersion: 1);
|
||||
|
||||
SshKeyCipher.TryOpen(sealedHost, vaultKey, entityId, itemVersion: 1).ShouldBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AKey_RoundTripsThroughTheCipher()
|
||||
{
|
||||
var vaultKey = RandomNumberGenerator.GetBytes(32);
|
||||
|
||||
var entityId = Guid.CreateVersion7();
|
||||
var key = NewKey();
|
||||
|
||||
var payload = SshKeyCipher.Seal(key, vaultKey, entityId, keyGeneration: 1, itemVersion: 3);
|
||||
var opened = SshKeyCipher.TryOpen(payload, vaultKey, entityId, itemVersion: 3);
|
||||
|
||||
opened.ShouldNotBeNull();
|
||||
opened.Key.ShouldBe(key);
|
||||
opened.SchemaVersion.ShouldBe(SshKeySecretCodec.CurrentSchemaVersion);
|
||||
opened.IsReadOnly.ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AKeySealedAtOneVersion_DoesNotOpenAtAnother()
|
||||
{
|
||||
// The item version is in the AAD, which is what stops a server rolling a row back to earlier
|
||||
// ciphertext. Asserted for keys as well as hosts because it is the property most easily lost by
|
||||
// copying a cipher and adjusting the wrong argument.
|
||||
var vaultKey = RandomNumberGenerator.GetBytes(32);
|
||||
|
||||
var entityId = Guid.CreateVersion7();
|
||||
|
||||
var payload = SshKeyCipher.Seal(NewKey(), vaultKey, entityId, keyGeneration: 1, itemVersion: 2);
|
||||
|
||||
SshKeyCipher.TryOpen(payload, vaultKey, entityId, itemVersion: 3).ShouldBeNull();
|
||||
}
|
||||
|
||||
private static SshKeySecret NewKey() => new()
|
||||
{
|
||||
Label = "deploy",
|
||||
PrivateKeyPem = "-----BEGIN OPENSSH PRIVATE KEY-----\nnot-a-real-key\n-----END OPENSSH PRIVATE KEY-----",
|
||||
Passphrase = "a passphrase",
|
||||
PublicKey = "ssh-ed25519 AAAAC3Nz deploy@example",
|
||||
Notes = "used by CI",
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user