Public Access
Keep host key trust in the vault, and make it withdrawable
A fingerprint approved once is now approved on every machine and survives a
restart, because host key trust is a vault item type rather than a dictionary
that dies with the process. InMemoryKnownHostStore was what shipped, so the user
was asked to verify a fingerprint on every single connection — which is the gap
most likely to train somebody to click through the one warning that actually
matters. A warning that appears when nothing is wrong teaches that nothing is
ever wrong.
The fourth item type, and like the third it cost no sync logic: a row, an EF
configuration, a migration, a server kind; a secret, a codec, a merge, a cipher,
a repository facade and a session property. One row in the client registry. The
reconciler, the mirror, the repository, the outbox and the pull filter were not
touched. SyncEntityType.KnownHostKey and AadResourceType.KnownHostKey were
already reserved, so neither the contract nor docs/crypto.md changed.
One item per (host, port, algorithm), because a server legitimately offers
several host keys and which one gets negotiated is not ours to predict. Pinning
per endpoint would make an algorithm change indistinguishable from an attack.
The label is derived rather than stored, which is the one place this type
departs from the other three. A user never names a pin — there is nothing to
name it after but the three fields it already has — and a stored label is a
second copy of data that can disagree with the first after a merge. Relabel
returns the secret unchanged, and says why.
The store answers the handshake without touching the disk. SshNetConnectionFactory
calls FindAsync from inside SSH.NET's synchronous HostKeyReceived event, over
.GetAwaiter().GetResult(), which cannot be avoided; doing SQLite I/O plus an AEAD
open per lookup there would put the handshake behind the cache. So decryption
happens in OpenAsync and RefreshAsync — on unlock and after each sync pass,
exactly where the host and key lists already reload — and FindAsync is a
dictionary read under a lock with no await inside it.
That snapshot is where the one real bug in this change lived. Install originally
merged the live pins over the freshly loaded snapshot, to protect a TrustAsync
that had landed while the read was in flight. It would also have resurrected
every pin the user had just forgotten, and stopped a withdrawal made on another
machine from ever taking effect — the store would have healed the deletion back
into existence on every refresh. Replacing wholesale and discarding the read
instead is correct because writes are the rare case: every write bumps a
generation counter, and a refresh whose stamp is stale throws itself away rather
than winning. Nothing found this but reading the method again; it is the kind of
mistake that passes every test written before it, because the test that catches
it is the one the bug tells you to write.
Forgetting is new, and persistence is what made it mandatory rather than
convenient. A mismatch is a hard refusal with no way to continue — deliberately,
and that stays — so pinning a key permanently is also a way to make a
legitimately rebuilt server permanently unreachable. Before this change the pin
died at exit and the problem solved itself; now it does not.
ForgetAsync drops every algorithm for an endpoint, and it is reachable from the
host editor rather than from the warning. Putting it on the mismatch banner would
have made it two clicks from "this may be an attack" to "connect anyway", which
is the affordance the hard refusal exists to deny. The banner already promised
the key could be removed in the host's settings; that promise is now true and
points at the button.
Trust recorded on another machine becomes visible at the next sync pass, not
immediately, and that is a decision rather than an oversight. The failure it
produces is a first-contact prompt for a host a colleague approved a minute ago:
answerable, and self-correcting on the next pass. The opposite trade — polling
the vault on the handshake thread to close a one-minute window — buys nothing
and costs the property above. The dangerous direction is not reachable at all: a
pin recorded here enters the snapshot as part of recording it, so a refresh can
never discard a local trust decision.
The server learns nothing, and this is the item type where the temptation was
real. A plaintext host column would let a known-hosts screen sort and page
without decrypting anything, and it would hand the operator the map of every
user's estate — assembled, as these things are, out of facts that are each
individually harmless. A host row concedes an address only when relay is
switched on and the database refuses to store one otherwise (ADR 0004); there is
no equivalent excuse here. The table has no column to put one in, and the EF
configuration says so where somebody adding it would be standing.
Two things about the migration in this commit are worth knowing, because both
came out of getting it wrong.
It was hand-written first, including its .Designer.cs, and that version is not
what is here. Verifying it turned up something that had been quietly assumed:
Migration_AppliedCleanly_WithNoPendingModelChanges does not check the model
snapshot. It asserts that migrations applied and that none are pending, which a
wrong snapshot satisfies perfectly — the snapshot only matters as the diff base
for the *next* migrations add, so an incorrect one passes the whole suite and
corrupts the following migration instead. The real check is to generate a
throwaway migration and confirm its Up and Down come out empty. They did, and
the generated designer was byte-identical to the transcribed one across all 1255
lines, so the hand-written work was in fact correct.
Then dotnet ef migrations remove --no-build deleted the wrong migration. With
--no-build the tool reads the previously compiled assembly rather than the files
on disk, and the probe had just changed which migration was last, so it removed
AddKnownHostKeyItem and reverted the snapshot. That turned out to leave exactly
the right diff base, so the migration here is EF's own output rather than a
transcription — a better outcome than the one that was interrupted, arrived at
by accident. Never pass --no-build to migrations remove.
Mutation tested, all three sabotages detected: dropping the algorithm from
KnownHostIdentity.For, merging instead of replacing in Install, and pointing
KnownHostKeyCipher at PortForward — which is what a cast from the wire enum's 10
would silently produce. Each is caught both by an assertion about the mechanism
and by a behavioural test that never mentions it; the resource-type sabotage is
caught by the table from d10a38d and nothing else, which is what that table is
for.
The end-to-end slice now approves the real sshd's host key through the vault,
pushes it, and reads it back on the second simulated machine — including a check
that the server learned no address, and that the second machine answers null for
an algorithm never offered.
845 tests green. Zero warnings, dotnet format clean.
Three things are deliberately not fixed. A tombstone queued over a create that
was never pushed is refused by the server as Invalid and parked; that is
pre-existing for all four item types, and the fix belongs in
VaultItemRepository.DeleteAsync rather than here. Deleting a host, or changing
its address, orphans its pins — both are correct as trust decisions, since a pin
describes an endpoint and not a bookmark, but nothing surfaces the leftovers.
And there is no interface listing pins at all: trust is created at the connect
prompt and withdrawn in the host editor. A known-hosts list is where the orphans
would become visible, and it wants the vault column rework first, for the same
reason the credential editor does.
This commit is contained in:
@@ -27,7 +27,8 @@ internal sealed record MergedItem<TSecret>(
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The reconciler holds the six answers a collision can have — merge, adopt, resurrect, abandon, park,
|
||||
/// refuse — and every one of them is identical for a host, an SSH key and a credential. Only the encoding,
|
||||
/// refuse — and every one of them is identical for a host, an SSH key, a credential and a pinned host key.
|
||||
/// Only the encoding,
|
||||
/// the merge and the plaintext columns differ, and those arrive through here. A second copy of the
|
||||
/// reconciler per item type is the alternative, and it is not a real one: the file's whole premise is
|
||||
/// that the pull and push paths must answer the same situation the same way, and two copies would drift
|
||||
@@ -116,6 +117,9 @@ internal static class ItemKinds
|
||||
|
||||
(SyncEntityType.Credential, static (outbox, conflicts, keyring) =>
|
||||
new ItemReconciler<CredentialSecret>(CredentialKind.Instance, outbox, conflicts, keyring)),
|
||||
|
||||
(SyncEntityType.KnownHostKey, static (outbox, conflicts, keyring) =>
|
||||
new ItemReconciler<KnownHostSecret>(KnownHostKeyKind.Instance, outbox, conflicts, keyring)),
|
||||
];
|
||||
|
||||
/// <summary>The types to ask the server for, in a fixed order.</summary>
|
||||
@@ -316,3 +320,88 @@ internal sealed class CredentialKind : IItemKind<CredentialSecret>
|
||||
return secret with { Label = label };
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Known host keys.</summary>
|
||||
internal sealed class KnownHostKeyKind : IItemKind<KnownHostSecret>
|
||||
{
|
||||
internal static KnownHostKeyKind Instance { get; } = new();
|
||||
|
||||
/// <inheritdoc />
|
||||
public SyncEntityType EntityType => SyncEntityType.KnownHostKey;
|
||||
|
||||
/// <summary>
|
||||
/// What to call one of these to a person.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// "Known host key" rather than "host key", because a user who reads "this host key could not be
|
||||
/// decrypted" would go looking at the host they are connecting to. The item is the record of a decision
|
||||
/// they made about that host, and the noun has to say so.
|
||||
/// </remarks>
|
||||
/// <inheritdoc />
|
||||
public string Noun => "known host key";
|
||||
|
||||
/// <inheritdoc />
|
||||
public OpenedItem<KnownHostSecret>? TryOpen(
|
||||
EncryptedPayload payload,
|
||||
ReadOnlySpan<byte> vaultKey,
|
||||
Guid entityId,
|
||||
int itemVersion)
|
||||
{
|
||||
var document = KnownHostKeyCipher.TryOpen(payload, vaultKey, entityId, itemVersion);
|
||||
|
||||
return document is null
|
||||
? null
|
||||
: new OpenedItem<KnownHostSecret>(document.KnownHost, document.IsReadOnly);
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public EncryptedPayload Seal(
|
||||
KnownHostSecret secret,
|
||||
ReadOnlySpan<byte> vaultKey,
|
||||
Guid entityId,
|
||||
uint keyGeneration,
|
||||
int itemVersion) =>
|
||||
KnownHostKeyCipher.Seal(secret, vaultKey, entityId, keyGeneration, itemVersion);
|
||||
|
||||
/// <summary>
|
||||
/// Nothing, and here the temptation is the strongest of the four.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// This item holds an address and a fingerprint, and neither is confidential in itself: the operator
|
||||
/// published the fingerprint and the address is one the server may already hold for a relay-enabled host.
|
||||
/// Handing them over anyway would tell the operator which machines each user actually connects to and
|
||||
/// when they first did — a map of the estate assembled out of individually harmless facts, for columns
|
||||
/// that do not exist and nothing that would read them. See ADR 0004.
|
||||
/// </remarks>
|
||||
/// <inheritdoc />
|
||||
public SyncPlaintextFields? Fields(KnownHostSecret secret) => null;
|
||||
|
||||
/// <inheritdoc />
|
||||
public MergedItem<KnownHostSecret> Merge(
|
||||
KnownHostSecret ancestor,
|
||||
KnownHostSecret local,
|
||||
KnownHostSecret remote)
|
||||
{
|
||||
var merged = KnownHostSecretMerge.Merge(ancestor, local, remote);
|
||||
|
||||
return new MergedItem<KnownHostSecret>(merged.Merged, merged.Conflicts);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The pin unchanged, because a pin has no name of its own to change.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The only caller is the reconciler's resurrection path, which renames rescued content so a user can see
|
||||
/// what happened to it. <see cref="KnownHostSecret.Label"/> is derived from the host, port and algorithm
|
||||
/// the pin is about, so there is nothing here that a rename could move: renaming it would mean claiming
|
||||
/// the pin is about a different host. The resurrected item still gets its own id and still produces a
|
||||
/// conflict notice, so the event is visible — the notice simply names the pin the same way twice.
|
||||
/// </remarks>
|
||||
/// <inheritdoc />
|
||||
public KnownHostSecret Relabel(KnownHostSecret secret, string label)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(secret);
|
||||
|
||||
return secret;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,131 @@
|
||||
using System.Security.Cryptography;
|
||||
using DodoSSH.Client.Domain;
|
||||
using DodoSSH.Contracts;
|
||||
using DodoSSH.Crypto;
|
||||
|
||||
namespace DodoSSH.Client.Sync;
|
||||
|
||||
/// <summary>
|
||||
/// Turns a known host key into an item payload and back.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Mirrors <see cref="HostCipher"/> exactly, including the rule that a payload is sealed at the version the
|
||||
/// server <em>will</em> assign rather than the one it replaces — see <see cref="SyncVersions"/>.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>The resource type is the one thing not to copy.</b> As with <see cref="CredentialCipher"/>, the AAD
|
||||
/// binds it and the two enums that name item types do not agree: <c>SyncEntityType.KnownHostKey</c> is 10
|
||||
/// while <c>CryptoSpec.AadResourceType.KnownHostKey</c> is 11, because the crypto enum carries None, User,
|
||||
/// Device and Vault ahead of the item types. Casting one to the other would seal a pin under the resource
|
||||
/// type for a <em>port forward</em> — which encrypts perfectly, decrypts perfectly on the machine that wrote
|
||||
/// it, and is a specification violation nothing would notice until an interoperating client refused the item.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public static class KnownHostKeyCipher
|
||||
{
|
||||
private const CryptoSpec.AadResourceType Resource = CryptoSpec.AadResourceType.KnownHostKey;
|
||||
|
||||
/// <summary>Encrypts a pin.</summary>
|
||||
/// <param name="knownHost">The pin. Must be valid for storage.</param>
|
||||
/// <param name="vaultKey">The vault key, which the data key is wrapped under.</param>
|
||||
/// <param name="entityId">The item id, which the AAD binds.</param>
|
||||
/// <param name="keyGeneration">The vault's current key generation.</param>
|
||||
/// <param name="itemVersion">The version this payload will hold once the server accepts it.</param>
|
||||
public static EncryptedPayload Seal(
|
||||
KnownHostSecret knownHost,
|
||||
ReadOnlySpan<byte> vaultKey,
|
||||
Guid entityId,
|
||||
uint keyGeneration,
|
||||
int itemVersion)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(knownHost);
|
||||
ArgumentOutOfRangeException.ThrowIfLessThan(itemVersion, 1);
|
||||
|
||||
var plaintext = KnownHostSecretCodec.Encode(knownHost);
|
||||
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);
|
||||
|
||||
// Wiped like every other payload in this folder, and for a smaller reason than the others: a
|
||||
// fingerprint is published on purpose, so nothing in here is a secret in the sense a password is.
|
||||
// What the buffer does hold is the address of a machine this user reaches, and leaving that in a
|
||||
// pooled buffer for nothing would be a gratuitous difference from the neighbouring ciphers.
|
||||
CryptographicOperations.ZeroMemory(plaintext);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Decrypts a pin.</summary>
|
||||
/// <inheritdoc cref="HostCipher.TryOpen" path="/returns" />
|
||||
public static KnownHostSecretDocument? TryOpen(
|
||||
EncryptedPayload payload,
|
||||
ReadOnlySpan<byte> 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 KnownHostSecretCodec.TryDecode(plaintext, out var document) ? document : null;
|
||||
}
|
||||
finally
|
||||
{
|
||||
CryptographicOperations.ZeroMemory(plaintext);
|
||||
}
|
||||
}
|
||||
finally
|
||||
{
|
||||
CryptographicOperations.ZeroMemory(dataKey);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,51 @@
|
||||
using DodoSSH.Client.Domain;
|
||||
using DodoSSH.Client.Storage;
|
||||
|
||||
namespace DodoSSH.Client.Sync;
|
||||
|
||||
/// <summary>
|
||||
/// The host keys this vault trusts, decrypted, with unpushed local changes laid over them.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The fourth facade over the same generic repository, and it needed no new sync logic at all — which was the
|
||||
/// point of the item-kind seam.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Nothing here is on the SSH handshake path.</b> Listing decrypts every pin in the vault, and the
|
||||
/// handshake asks about host key trust from inside a synchronous SSH.NET event where a decryption per lookup
|
||||
/// would be I/O and AEAD work on the thread completing the key exchange. <c>VaultKnownHostStore</c> exists to
|
||||
/// keep those apart: it reads through here when a vault opens and after each synchronisation pass, and answers
|
||||
/// the handshake from an in-memory snapshot.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class KnownHostRepository(ItemStore items, OutboxStore outbox, VaultKeyring keyring)
|
||||
{
|
||||
private readonly VaultItemRepository<KnownHostSecret> knownHosts =
|
||||
new(KnownHostKeyKind.Instance, items, outbox, keyring);
|
||||
|
||||
/// <inheritdoc cref="VaultItemRepository{TSecret}.ListAsync" />
|
||||
public Task<ItemListing<KnownHostSecret>> ListAsync(
|
||||
Guid vaultId,
|
||||
CancellationToken cancellationToken) =>
|
||||
knownHosts.ListAsync(vaultId, cancellationToken);
|
||||
|
||||
/// <inheritdoc cref="VaultItemRepository{TSecret}.CreateAsync" />
|
||||
public Task<Guid> CreateAsync(
|
||||
Guid vaultId,
|
||||
KnownHostSecret knownHost,
|
||||
CancellationToken cancellationToken) =>
|
||||
knownHosts.CreateAsync(vaultId, knownHost, cancellationToken);
|
||||
|
||||
/// <inheritdoc cref="VaultItemRepository{TSecret}.UpdateAsync" />
|
||||
public Task UpdateAsync(
|
||||
Guid vaultId,
|
||||
Guid entityId,
|
||||
KnownHostSecret knownHost,
|
||||
CancellationToken cancellationToken) =>
|
||||
knownHosts.UpdateAsync(vaultId, entityId, knownHost, cancellationToken);
|
||||
|
||||
/// <inheritdoc cref="VaultItemRepository{TSecret}.DeleteAsync" />
|
||||
public Task DeleteAsync(Guid vaultId, Guid entityId, CancellationToken cancellationToken) =>
|
||||
knownHosts.DeleteAsync(vaultId, entityId, cancellationToken);
|
||||
}
|
||||
Reference in New Issue
Block a user