diff --git a/src/DodoSSH.Client.Domain/SshKeySecret.cs b/src/DodoSSH.Client.Domain/SshKeySecret.cs new file mode 100644 index 0000000..bd6b22b --- /dev/null +++ b/src/DodoSSH.Client.Domain/SshKeySecret.cs @@ -0,0 +1,113 @@ +using System.Diagnostics.CodeAnalysis; + +namespace DodoSSH.Client.Domain; + +/// +/// An SSH key pair as the user sees it, decrypted. +/// +/// +/// +/// The private key is the most valuable thing this product stores, and it is held here as an ordinary +/// string, in managed memory, exactly like and every passphrase in the +/// client. That is a deliberate choice rather than an oversight, and it is worth being plain about what it +/// does and does not give you. +/// +/// +/// It does not give you a zeroable buffer. A .NET string cannot be wiped, so the key material lives until +/// the garbage collector reuses the memory, and a process dump taken in the meantime contains it. The +/// alternative — libsodium's guarded memory, which DodoSSH.Crypto uses for the vault keys — was +/// considered and rejected here for one reason: the passphrase protecting this key, the password on the next +/// item, and the decoded JSON the codec produced are all strings on the same heap. Protecting one field +/// among them would be a measure that reads as security and buys nothing, and the honest place for that +/// effort is the threat this design actually addresses, which is the server and the disk rather than another +/// process running as the same user. See LocalCacheProtector, which says the same thing about the +/// cache. +/// +/// +/// What it does give you: the key never reaches the disk in plaintext and never reaches the server at all. +/// It is sealed under the vault key before it leaves this type, and SSH.NET is handed it through a +/// MemoryStream rather than a file, so there is no temporary key file to leak or forget. +/// +/// +public sealed record SshKeySecret +{ + /// What the user calls this key. + public required string Label { get; init; } + + /// + /// The private key, in the armoured form ssh-keygen writes. + /// + /// + /// Stored verbatim, including its header and trailer. Not reformatted, not re-encoded, not normalised: + /// OpenSSH, PKCS#1 and PKCS#8 all round-trip through here untouched, and a client that rewrote them + /// would eventually rewrite one it did not fully understand. + /// + public required string PrivateKeyPem { get; init; } + + /// + /// The passphrase protecting the private key, when it has one. + /// + /// + /// Kept with the key rather than typed per connection, which is the entire point of a vault: the + /// passphrase defends the key file on a disk, and inside a vault the key is not on a disk. Storing both + /// together means the vault passphrase is what protects them, which is the guarantee this product is + /// built to make. A user who wants the second factor can leave this null and be prompted. + /// + public string? Passphrase { get; init; } + + /// + /// The public half, in authorized_keys form, when it is known. + /// + /// + /// Not derived from the private key, and deliberately optional. Deriving it means parsing every key + /// format this type accepts verbatim, which is the thing above that it declines to do. It is worth + /// keeping because installing a key on a host needs exactly this line, and a user who imported only a + /// private key should be told the public half is missing rather than have one silently reconstructed. + /// + public string? PublicKey { get; init; } + + /// Free text. + public string? Notes { get; init; } + + /// + /// Whether this key is storable, and why not if it is not. + /// + /// + /// The public-key mix-up is checked explicitly because it is the mistake a person actually makes: + /// ssh-keygen writes two files whose names differ by four characters, and pasting the wrong one + /// produces a vault item that looks fine and fails at connection time with an authentication error that + /// says nothing about which file you chose. + /// + public bool TryValidate([NotNullWhen(false)] out string? error) + { + if (string.IsNullOrWhiteSpace(Label)) + { + error = "A key needs a name."; + return false; + } + + if (string.IsNullOrWhiteSpace(PrivateKeyPem)) + { + error = "A key needs its private key material."; + return false; + } + + var material = PrivateKeyPem.TrimStart(); + + if (material.StartsWith("ssh-", StringComparison.Ordinal) + || material.StartsWith("ecdsa-", StringComparison.Ordinal)) + { + error = "That is a public key. Paste the private key — the file without the .pub extension."; + return false; + } + + if (!material.StartsWith("-----BEGIN", StringComparison.Ordinal)) + { + error = "That does not look like a private key; it should begin with \"-----BEGIN\"."; + return false; + } + + error = null; + return true; + } +} diff --git a/src/DodoSSH.Client.Domain/SshKeySecretCodec.cs b/src/DodoSSH.Client.Domain/SshKeySecretCodec.cs new file mode 100644 index 0000000..e8b147d --- /dev/null +++ b/src/DodoSSH.Client.Domain/SshKeySecretCodec.cs @@ -0,0 +1,126 @@ +using System.Diagnostics.CodeAnalysis; +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace DodoSSH.Client.Domain; + +/// A decoded SSH key payload, together with the schema version it was written at. +/// The key. +/// The version the writing client used. +public sealed record SshKeySecretDocument(SshKeySecret Key, int SchemaVersion) +{ + /// + /// + /// The consequence is sharper for a key than for a host. Re-encoding an item written by a newer client + /// drops the fields this build has no concept of — and if one of those fields were, say, a certificate + /// or a second key format, the item would still decrypt, still look complete, and no longer + /// authenticate. Refusing to write is the only safe answer. + /// + public bool IsReadOnly => SchemaVersion > SshKeySecretCodec.CurrentSchemaVersion; +} + +/// +/// Encodes and decodes the plaintext inside an SSH key item's encrypted payload. +/// +/// +/// Mirrors , for the same reasons and with the same guarantees: JSON so a field +/// can be added without a migration, deterministic property order so an unchanged key does not look like a +/// change to the sync engine, and a separate mutable document type so a decode failure cannot produce a +/// half-built key that looks valid downstream. +/// +public static class SshKeySecretCodec +{ + /// The schema version this build writes. + public const int CurrentSchemaVersion = 1; + + /// Serialises a key to the bytes that get sealed. + /// The key is not valid for storage. + public static byte[] Encode(SshKeySecret key) + { + ArgumentNullException.ThrowIfNull(key); + + if (!key.TryValidate(out var error)) + { + throw new ArgumentException(error, nameof(key)); + } + + var document = new SshKeyPayloadDocument + { + SchemaVersion = CurrentSchemaVersion, + Label = key.Label, + PrivateKeyPem = key.PrivateKeyPem, + Passphrase = key.Passphrase, + PublicKey = key.PublicKey, + Notes = key.Notes, + }; + + return JsonSerializer.SerializeToUtf8Bytes( + document, SshKeyPayloadJsonContext.Default.SshKeyPayloadDocument); + } + + /// Parses a decrypted payload. + /// + public static bool TryDecode( + ReadOnlySpan payload, + [NotNullWhen(true)] out SshKeySecretDocument? document) + { + document = null; + + SshKeyPayloadDocument? parsed; + try + { + parsed = JsonSerializer.Deserialize( + payload, SshKeyPayloadJsonContext.Default.SshKeyPayloadDocument); + } + catch (JsonException) + { + return false; + } + + if (parsed is null || parsed.SchemaVersion < 1) + { + return false; + } + + var candidate = new SshKeySecret + { + Label = parsed.Label ?? string.Empty, + PrivateKeyPem = parsed.PrivateKeyPem ?? string.Empty, + Passphrase = parsed.Passphrase, + PublicKey = parsed.PublicKey, + Notes = parsed.Notes, + }; + + if (!candidate.TryValidate(out _)) + { + return false; + } + + document = new SshKeySecretDocument(candidate, parsed.SchemaVersion); + return true; + } +} + +/// The serialised shape. Mutable and nullable because it models untrusted input. +/// +internal sealed class SshKeyPayloadDocument +{ + public int SchemaVersion { get; set; } + + public string? Label { get; set; } + + public string? PrivateKeyPem { get; set; } + + public string? Passphrase { get; set; } + + public string? PublicKey { get; set; } + + public string? Notes { get; set; } +} + +[JsonSourceGenerationOptions( + PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + UnmappedMemberHandling = JsonUnmappedMemberHandling.Skip)] +[JsonSerializable(typeof(SshKeyPayloadDocument))] +internal sealed partial class SshKeyPayloadJsonContext : JsonSerializerContext; diff --git a/src/DodoSSH.Client.Domain/SshKeySecretMerge.cs b/src/DodoSSH.Client.Domain/SshKeySecretMerge.cs new file mode 100644 index 0000000..a6a8cd6 --- /dev/null +++ b/src/DodoSSH.Client.Domain/SshKeySecretMerge.cs @@ -0,0 +1,120 @@ +namespace DodoSSH.Client.Domain; + +/// The merged key, and everything that had to be overridden to produce it. +/// The key to store and push. +/// Empty when the two sides were reconcilable field by field. +public sealed record SshKeyMergeResult( + SshKeySecret Merged, + IReadOnlyList Conflicts) +{ + /// Whether anything had to be overridden. + public bool HasConflicts => Conflicts.Count > 0; +} + +/// +/// Merges two divergent versions of an SSH key against the version they both started from. +/// +/// +/// +/// Every field is a scalar, so this is simpler than the host merge — there is no per-name map to reconcile +/// and no ordered list whose order is its meaning. It reuses rather than +/// declaring a parallel type: the conflict log, the storage that persists it and the interface that shows it +/// are all shared, and a second record with identical members would have to be mapped to the first at every +/// boundary. +/// +/// +/// The conflict detail never carries key material. A host conflict shows both values so the user can +/// put back the one that lost; doing that for a private key would write the discarded key into the conflict +/// log, which is a copy of a secret in a place designed to be read rather than used — and the log is +/// deliberately retained after acknowledgement. So the private key and its passphrase report only +/// that they differed. This is the one place where showing less is the safer answer, and it costs +/// the user nothing they can act on: two different private keys are not something anyone reconciles by +/// reading them side by side. +/// +/// +public static class SshKeySecretMerge +{ + /// Produces the merged key. + /// The version both sides branched from. + /// The pending local version. + /// The server's current version. + public static SshKeyMergeResult Merge(SshKeySecret ancestor, SshKeySecret local, SshKeySecret remote) + { + ArgumentNullException.ThrowIfNull(ancestor); + ArgumentNullException.ThrowIfNull(local); + ArgumentNullException.ThrowIfNull(remote); + + var conflicts = new List(); + + var merged = new SshKeySecret + { + // Null-forgiving on the two required fields, as HostSecretMerge does for the same reason: the + // merge returns one of the three inputs, and all three are non-null here by construction. + Label = Shown( + nameof(SshKeySecret.Label), ancestor.Label, local.Label, remote.Label, conflicts)!, + PrivateKeyPem = Redacted( + nameof(SshKeySecret.PrivateKeyPem), + ancestor.PrivateKeyPem, + local.PrivateKeyPem, + remote.PrivateKeyPem, + conflicts)!, + Passphrase = Redacted( + nameof(SshKeySecret.Passphrase), + ancestor.Passphrase, + local.Passphrase, + remote.Passphrase, + conflicts), + PublicKey = Shown( + nameof(SshKeySecret.PublicKey), + ancestor.PublicKey, + local.PublicKey, + remote.PublicKey, + conflicts), + Notes = Shown( + nameof(SshKeySecret.Notes), ancestor.Notes, local.Notes, remote.Notes, conflicts), + }; + + return new SshKeyMergeResult(merged, conflicts); + } + + /// A field whose overridden value is safe to show. + private static string? Shown( + string name, + string? ancestor, + string? local, + string? remote, + List conflicts) => + Resolve(name, ancestor, local, remote, conflicts, redact: false); + + /// A field whose overridden value must not be written to the conflict log. + private static string? Redacted( + string name, + string? ancestor, + string? local, + string? remote, + List conflicts) => + Resolve(name, ancestor, local, remote, conflicts, redact: true); + + private static string? Resolve( + string name, + string? ancestor, + string? local, + string? remote, + List conflicts, + bool redact) + { + var merge = ThreeWayMerge.Scalar(ancestor, local, remote, StringComparer.Ordinal); + + if (merge.IsConflicted) + { + conflicts.Add(new HostFieldConflict( + name, + MergeSide.Local, + redact ? "(kept the server's value)" : merge.Value, + redact ? "(a different value was discarded)" : merge.Discarded, + DiscardedWasRemoval: false)); + } + + return merge.Value; + } +} diff --git a/src/DodoSSH.Client.Sync/SshKeyCipher.cs b/src/DodoSSH.Client.Sync/SshKeyCipher.cs new file mode 100644 index 0000000..6a70d7d --- /dev/null +++ b/src/DodoSSH.Client.Sync/SshKeyCipher.cs @@ -0,0 +1,130 @@ +using System.Security.Cryptography; +using DodoSSH.Client.Domain; +using DodoSSH.Contracts; +using DodoSSH.Crypto; + +namespace DodoSSH.Client.Sync; + +/// +/// Turns an SSH key into an item payload and back. +/// +/// +/// +/// Mirrors exactly, including the rule that a payload is sealed at the version the +/// server will assign rather than the one it replaces — see . +/// +/// +/// The resource type is the one thing not to copy. The AAD binds it, and the two enums that name item +/// types do not agree: SyncEntityType.SshKey is 3 while CryptoSpec.AadResourceType.SshKey is 6, +/// because the crypto enum also carries None, User, Device and Vault ahead of the item types. Casting one to +/// the other would seal a private key under the resource type for a vault — which encrypts +/// perfectly, decrypts perfectly on the machine that wrote it, and is a specification violation that no test +/// would notice until an interoperating client refused the item. So it is named here as a constant, and +/// AadResourceTypeTests pins the pairing. +/// +/// +public static class SshKeyCipher +{ + private const CryptoSpec.AadResourceType Resource = CryptoSpec.AadResourceType.SshKey; + + /// Encrypts an SSH key. + /// The key. Must be valid for storage. + /// The vault key, which the data key is wrapped under. + /// The item id, which the AAD binds. + /// The vault's current key generation. + /// The version this payload will hold once the server accepts it. + public static EncryptedPayload Seal( + SshKeySecret key, + ReadOnlySpan vaultKey, + Guid entityId, + uint keyGeneration, + int itemVersion) + { + ArgumentNullException.ThrowIfNull(key); + ArgumentOutOfRangeException.ThrowIfLessThan(itemVersion, 1); + + var plaintext = SshKeySecretCodec.Encode(key); + var dataKey = ItemKeys.CreateDataKey(); + + try + { + var dataKeyId = Guid.CreateVersion7(); + + var wrappedDataKey = ItemKeys.WrapDataKey( + dataKey, vaultKey, Resource, entityId, keyGeneration, (uint)itemVersion); + + var envelope = ItemKeys.SealPayload( + dataKey, plaintext, Resource, entityId, dataKeyId, keyGeneration, (uint)itemVersion); + + return new EncryptedPayload( + envelope, wrappedDataKey, dataKeyId, keyGeneration, CryptoSpec.CurrentAadVersion); + } + finally + { + CryptographicOperations.ZeroMemory(dataKey); + + // The encoded key material, wiped. This is the one buffer in the client whose contents are a + // private key and which can actually be cleared — the strings the codec read it from cannot be. + CryptographicOperations.ZeroMemory(plaintext); + } + } + + /// Decrypts an SSH key. + /// + public static SshKeySecretDocument? TryOpen( + EncryptedPayload payload, + ReadOnlySpan vaultKey, + Guid entityId, + int itemVersion) + { + ArgumentNullException.ThrowIfNull(payload); + + if (itemVersion < 1 || payload.WrappedDataKey.Length == 0) + { + return null; + } + + var dataKey = ItemKeys.TryUnwrapDataKey( + vaultKey, + payload.WrappedDataKey, + Resource, + entityId, + payload.KeyGeneration, + (uint)itemVersion); + + if (dataKey is null) + { + return null; + } + + try + { + var plaintext = ItemKeys.TryOpenPayload( + dataKey, + payload.Envelope, + Resource, + entityId, + payload.DataKeyId, + payload.KeyGeneration, + (uint)itemVersion); + + if (plaintext is null) + { + return null; + } + + try + { + return SshKeySecretCodec.TryDecode(plaintext, out var document) ? document : null; + } + finally + { + CryptographicOperations.ZeroMemory(plaintext); + } + } + finally + { + CryptographicOperations.ZeroMemory(dataKey); + } + } +} diff --git a/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs b/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs new file mode 100644 index 0000000..6f4b69a --- /dev/null +++ b/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs @@ -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; + +/// +/// Each item type must be sealed under its own AAD resource type, and the two enums that name item types +/// deliberately do not agree. +/// +/// +/// +/// SyncEntityType lists only syncable items, so Host is 1 and SshKey is 3. +/// CryptoSpec.AadResourceType 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. +/// +/// +/// 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. +/// +/// +public sealed class AadResourceTypeTests +{ + /// + /// The pairing stated as a table. If AadResourceType is ever renumbered, this is what says so — + /// loudly, and before anything is written under the new numbers. + /// + [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."); + } + + /// + /// + /// 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. + /// + /// + /// 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. + /// + /// + [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."); + } + + /// Pins the host cipher the same way, since the two are now easy to confuse for each other. + [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", + }; +}