Bind an SSH key to a host instead of picking one per connection

A host now names the key it authenticates with, or none, as a field in its
encrypted payload — so the choice follows the host to every machine rather than
being made again each time somebody connects. The per-connection "Use key"
switch it replaces was a stopgap for not having this, and keeping both would
have left two mechanisms answering one question.

This is the first payload schema version bump, and it does not work the obvious
way. A host is written at the *lowest* schema version that can represent it: one
that binds a key is written at 2, one that does not is still written at 1, byte
for byte as it was before the field existed. The version is what makes an older
client refuse to edit an item, so stamping 2 unconditionally would mean
upgrading a single machine and renaming a single host made that host uneditable
on every machine that had not upgraded yet. Confining the cost to the hosts that
actually use the field is the difference between a team noticing a bump and a
team being blocked by one. HostSecretCodec states the rule so the next field
added follows it, and a test pins the version-1 bytes against a literal rather
than against the codec, because the claim is about history: every host already in
every vault has to re-encode to what it encoded before, or the first sync after
an upgrade would push the whole vault as changed.

A binding is an item id, not a copy of the key — a second copy of a private key
is one that goes stale — which means the reference can dangle when the key is
deleted on another machine. Both places that meets are handled the same way, by
refusing rather than falling back:

- Connecting to a host whose key is gone is refused outright. A host somebody
  deliberately set up for key-only access must not quietly start offering a
  password.
- Opening such a host in the editor keeps the binding, selected, labelled as
  missing. The quieter version of the same failure is someone editing the port
  and saving, silently converting the host to password authentication with
  nothing ever having said so.

Two things this found by being falsified:

- The merge was untested for the new field, and "just take the server's value"
  passed the entire suite — a local binding change would have been discarded with
  no conflict recorded. HostSecretMergeTests already had a test written for
  exactly this class of omission; it simply had not been extended.

- Adding a nullable field exposed a defect in HostSecretMerge.Field: it
  short-circuited when the discarded value was null, so the formatter never ran
  for the one case where null is a value rather than an absence, and a field
  whose absence has a name could not report it. Now the formatter always runs,
  and "no key" appears in the conflict log where an empty string used to.

Also fixes eight nullable warnings in SyncEndpointTests left by the server-side
SSH key commit, which had omitted the null-forgiving operator the rest of that
file uses. They were invisible until an unrelated change forced the project to
recompile.

The end-to-end slice now binds its host to its key, so a schema-version-2
payload goes through the real API, the real PostgreSQL and back out on a second
machine.

745 tests green. Zero warnings, dotnet format clean.
This commit is contained in:
2026-07-29 20:42:51 +02:00
parent e3fd3e1728
commit 70b3290a77
12 changed files with 523 additions and 105 deletions
+33
View File
@@ -57,6 +57,31 @@ public sealed record HostSecret : IVaultSecret
/// <summary>SSH directives, unique by name.</summary>
public HostOptions Options { get; init; } = HostOptions.Empty;
/// <summary>
/// The vault SSH key to authenticate with, or null to use a password.
/// </summary>
/// <remarks>
/// <para>
/// An item id rather than the key itself, because the key is a vault item in its own right and a copy
/// embedded here would be a second copy of a private key to keep in step — rotated in one place and
/// stale in the other. The cost is that the reference can dangle: the key may be deleted on another
/// machine while this host still names it. That is handled where it is noticed rather than prevented
/// here, and it is handled by refusing to connect, never by falling back to a password. Quietly sending
/// a password to a host the user had set up for key-only access is the one outcome worth ruling out.
/// </para>
/// <para>
/// Inside the encrypted payload, like everything else. It would have fitted the contract's plaintext
/// <c>RelatedId</c> column, and putting it there would tell the server which hosts share a key — a
/// graph of the user's infrastructure it has no need for.
/// </para>
/// <para>
/// This is the field that made the payload schema versioned in practice rather than in principle: see
/// <see cref="HostSecretCodec.CurrentSchemaVersion"/> for what a host carrying one means to an older
/// client, and why a host without one is still written at version 1.
/// </para>
/// </remarks>
public Guid? SshKeyId { get; init; }
/// <summary>
/// Whether this host may be dialled through the server relay.
/// </summary>
@@ -110,6 +135,14 @@ public sealed record HostSecret : IVaultSecret
return false;
}
if (SshKeyId == Guid.Empty)
{
// An empty id is not "no key" — that is null. It is a reference that can never resolve, and
// storing one would produce a host that refuses to connect with no way to see why.
reason = "An SSH key reference cannot be an empty id; use no key instead.";
return false;
}
reason = null;
return true;
}
+40 -3
View File
@@ -53,8 +53,14 @@ public sealed record HostSecretDocument(HostSecret Host, int SchemaVersion)
/// </remarks>
public static class HostSecretCodec
{
/// <summary>The schema version this build writes.</summary>
public const int CurrentSchemaVersion = 1;
/// <summary>The first version, and the one a host with no newer field is still written at.</summary>
public const int BaseSchemaVersion = 1;
/// <summary>The version that introduced <see cref="HostSecret.SshKeyId"/>.</summary>
public const int SshKeyIdSchemaVersion = 2;
/// <summary>The highest schema version this build can write.</summary>
public const int CurrentSchemaVersion = SshKeyIdSchemaVersion;
/// <summary>Serialises a host to the bytes that get sealed.</summary>
/// <exception cref="ArgumentException">The host is not valid for storage.</exception>
@@ -75,7 +81,7 @@ public static class HostSecretCodec
var document = new HostPayloadDocument
{
SchemaVersion = CurrentSchemaVersion,
SchemaVersion = SchemaVersionFor(host),
Label = host.Label,
Hostname = host.Hostname,
Port = host.Port,
@@ -84,12 +90,35 @@ public static class HostSecretCodec
JumpHostIds = [.. host.JumpHostIds],
Options = options,
RelayEnabled = host.RelayEnabled,
SshKeyId = host.SshKeyId,
};
return JsonSerializer.SerializeToUtf8Bytes(
document, HostPayloadJsonContext.Default.HostPayloadDocument);
}
/// <summary>
/// The lowest schema version that can represent this host without losing anything.
/// </summary>
/// <remarks>
/// <para>
/// Not simply <see cref="CurrentSchemaVersion"/>, and in a shared vault the difference is the whole
/// point. The version is what makes an older client treat an item as read-only, so stamping the newest
/// one unconditionally would mean that upgrading a single machine and then touching <em>any</em> host —
/// renaming it, changing a port — made that host uneditable on every machine that had not been upgraded
/// yet. Emitting the lowest version that loses nothing confines that cost to the hosts which actually
/// use the newer field.
/// </para>
/// <para>
/// The rule generalises, and the next field added should follow it: a host is written at the version
/// that introduced the newest field it actually carries. It also means the bytes for a host with no key
/// are identical to what this codec produced before <see cref="HostSecret.SshKeyId"/> existed, so
/// adding the field did not make every host in every vault look like a change to the sync engine.
/// </para>
/// </remarks>
private static int SchemaVersionFor(HostSecret host) =>
host.SshKeyId is null ? BaseSchemaVersion : SshKeyIdSchemaVersion;
/// <summary>
/// Parses a decrypted payload.
/// </summary>
@@ -154,6 +183,7 @@ public static class HostSecretCodec
JumpHostIds = JumpChain.Create(parsed.JumpHostIds ?? []),
Options = options,
RelayEnabled = parsed.RelayEnabled,
SshKeyId = parsed.SshKeyId,
};
if (!candidate.TryValidate(out _))
@@ -199,6 +229,13 @@ internal sealed class HostPayloadDocument
public SortedDictionary<string, string>? Options { get; set; }
public bool RelayEnabled { get; set; }
/// <remarks>
/// Last, deliberately. Property order is the serialisation order, so appending keeps the bytes for every
/// field that existed before this one byte-identical — and a null is omitted entirely, which is what
/// makes a host with no key encode exactly as it did before the field existed.
/// </remarks>
public Guid? SshKeyId { get; set; }
}
[JsonSourceGenerationOptions(
+21 -1
View File
@@ -103,6 +103,17 @@ public static class HostSecretMerge
remote.RelayEnabled,
conflicts,
static enabled => enabled ? "enabled" : "disabled"),
// The id is shown in a clash rather than redacted. It is not a secret — it names a vault item,
// it is not the key — and hiding it would leave the user unable to tell which of two keys the
// merge dropped.
SshKeyId = Field(
nameof(HostSecret.SshKeyId),
ancestor.SshKeyId,
local.SshKeyId,
remote.SshKeyId,
conflicts,
static id => id?.ToString() ?? "no key"),
};
return new HostMergeResult(merged, conflicts);
@@ -117,8 +128,17 @@ public static class HostSecretMerge
Field(name, ancestor, local, remote, conflicts, static value => value, StringComparer.Ordinal)!;
/// <remarks>
/// <para>
/// A scalar clash always overrides the local side — see <see cref="ThreeWayMerge"/> — so the
/// discarded side is fixed here rather than derived.
/// </para>
/// <para>
/// The formatter is handed the discarded value even when that value is null, and the null-forgiving
/// operator says why that is safe: a conflicted merge always has a discarded value, so a null here is a
/// nullable field whose discarded value was "unset" rather than a missing one. Short-circuiting on null
/// instead — which this did — meant the formatter never ran for exactly that case, so a field whose
/// absence has a name could not report it and the conflict log showed an empty string in its place.
/// </para>
/// </remarks>
private static T Field<T>(
string name,
@@ -137,7 +157,7 @@ public static class HostSecretMerge
name,
MergeSide.Local,
format(merge.Value),
merge.Discarded is null ? null : format(merge.Discarded),
format(merge.Discarded!),
DiscardedWasRemoval: false));
}