From 4c7f47c0262df3aaca380f932854ad457ec0b6a9 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Fri, 2 Oct 2026 13:16:30 +0200 Subject: [PATCH] Refuse a pinned host that answers with a key of another type, rather than asking whether to trust it Pins are stored per algorithm, so an endpoint pinned for ssh-ed25519 that was answered with only an ECDSA or RSA key found no pin and raised SshHostKeyUnknownException: the first-use question, where a pin exists to produce a hard refusal. Anybody in the middle needed nothing more than a key of a different type. The connection factory now reads every pin for the endpoint before the handshake, offers the pinned algorithms first so a server that still holds its pinned key negotiates it, and in the gate accepts a key under an unpinned algorithm only when its fingerprint matches one of the endpoint's pins (one RSA key signs as rsa-sha2-256 and rsa-sha2-512). Anything else is a mismatch. IKnownHostStore gains FindForEndpointAsync for this. --- .../VaultKnownHostStore.cs | 20 +++++ src/DodoSSH.Client.Ssh/HostKeyTrust.cs | 32 ++++++++ .../SshNetConnectionFactory.cs | 74 ++++++++++++++++++- .../HostKeyPinningTests.cs | 69 +++++++++++++++++ 4 files changed, 193 insertions(+), 2 deletions(-) create mode 100644 tests/DodoSSH.Client.Ssh.Tests/HostKeyPinningTests.cs diff --git a/src/DodoSSH.Client.Session/VaultKnownHostStore.cs b/src/DodoSSH.Client.Session/VaultKnownHostStore.cs index b9013de..a1ca38e 100644 --- a/src/DodoSSH.Client.Session/VaultKnownHostStore.cs +++ b/src/DodoSSH.Client.Session/VaultKnownHostStore.cs @@ -145,6 +145,26 @@ public sealed class VaultKnownHostStore : IKnownHostStore } } + /// + /// From the snapshot, like , and for the same reason. + public ValueTask> FindForEndpointAsync( + string host, + int port, + CancellationToken cancellationToken) + { + lock (gate) + { + IReadOnlyList found = pins.Values + .Where(pin => + KnownHostIdentity.Comparer.Equals(pin.Secret.Host, host) && pin.Secret.Port == port) + .Select(pin => new HostKeyPresentation( + pin.Secret.Host, pin.Secret.Port, pin.Secret.Algorithm, pin.Secret.Fingerprint)) + .ToArray(); + + return ValueTask.FromResult(found); + } + } + /// /// /// The vault is not open, so there is nowhere to record trust. diff --git a/src/DodoSSH.Client.Ssh/HostKeyTrust.cs b/src/DodoSSH.Client.Ssh/HostKeyTrust.cs index 3faf640..eea44f7 100644 --- a/src/DodoSSH.Client.Ssh/HostKeyTrust.cs +++ b/src/DodoSSH.Client.Ssh/HostKeyTrust.cs @@ -64,6 +64,22 @@ public interface IKnownHostStore /// ValueTask FindAsync(string host, int port, string algorithm, CancellationToken cancellationToken); + /// Returns every key pinned for one endpoint, whatever the algorithm. + /// + /// + /// ◆ What stops a pin being sidestepped by changing algorithm. answers + /// per algorithm, so on its own a host pinned for ssh-ed25519 that is answered by somebody offering + /// only ecdsa-sha2-nistp256 looks like a host never seen before — and "never seen before" is a + /// question the user is asked, where "changed" is a refusal. Knowing the endpoint has pins at all is what + /// lets the connection factory tell the two apart; see SshNetConnectionFactory. + /// + /// The same rule as : no I/O. + /// + ValueTask> FindForEndpointAsync( + string host, + int port, + CancellationToken cancellationToken); + /// Records a host key as trusted. ValueTask TrustAsync(HostKeyPresentation presentation, CancellationToken cancellationToken); @@ -156,6 +172,22 @@ public sealed class InMemoryKnownHostStore : IKnownHostStore } } + /// + public ValueTask> FindForEndpointAsync( + string host, + int port, + CancellationToken cancellationToken) + { + lock (gate) + { + IReadOnlyList found = pins.Values + .Where(pin => KnownHostIdentity.Comparer.Equals(pin.Host, host) && pin.Port == port) + .ToArray(); + + return ValueTask.FromResult(found); + } + } + /// public ValueTask TrustAsync(HostKeyPresentation presentation, CancellationToken cancellationToken) { diff --git a/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs b/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs index eac59f8..c8a5a5e 100644 --- a/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs +++ b/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs @@ -187,7 +187,13 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts) IProgress? progress, CancellationToken cancellationToken) { - var gate = new HostKeyGate(knownHosts, request, progress, cancellationToken); + var endpointPins = await knownHosts + .FindForEndpointAsync(request.Host, request.Port, cancellationToken) + .ConfigureAwait(false); + + PreferPinnedAlgorithms(client.ConnectionInfo, endpointPins); + + var gate = new HostKeyGate(knownHosts, request, endpointPins, progress, cancellationToken); client.HostKeyReceived += gate.OnHostKeyReceived; @@ -217,17 +223,73 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts) return gate; } + /// + /// Offers the host key algorithms this endpoint has pins for ahead of every other. + /// + /// + /// + /// The server takes the first algorithm in the client's list that it also supports, so this is what + /// makes a server that still holds its pinned key negotiate that key — rather than, after it gained an + /// ed25519 key alongside the rsa one that was pinned, a key nobody has approved. Without it + /// the gate below would have to choose between refusing a legitimate server and prompting for one that + /// may not be, which is exactly the choice it must not be left with. + /// + /// + /// Reordered rather than restricted, so a host whose pinned keys are all gone still completes the key + /// exchange and reaches the gate, which reports it as a changed key with both fingerprints. Restricting + /// would fail the exchange with "no matching host key algorithm", which is true and tells nobody what + /// happened. Cleared and refilled because that is the only way a dictionary promises an order. + /// + /// + private static void PreferPinnedAlgorithms( + ConnectionInfo info, + IReadOnlyList pins) + { + if (pins.Count == 0) + { + return; + } + + var pinned = pins + .Select(pin => pin.Algorithm) + .ToHashSet(StringComparer.Ordinal); + + var ordered = info.HostKeyAlgorithms + .OrderBy(algorithm => pinned.Contains(algorithm.Key) ? 0 : 1) + .ToList(); + + info.HostKeyAlgorithms.Clear(); + + foreach (var algorithm in ordered) + { + info.HostKeyAlgorithms.Add(algorithm.Key, algorithm.Value); + } + } + /// /// Decides host key trust during the handshake, and remembers enough to explain a refusal. /// /// + /// /// Separate from the connect method because the decision is the security-relevant part and reads /// better on its own: pinned and equal accepts, pinned and different refuses as a mismatch, /// unpinned refuses as unknown. There is no fourth branch, and there is no prompt. + /// + /// + /// ◆ "Pinned" means pinned for this endpoint, not for this algorithm. Pins are stored per + /// algorithm, and an endpoint whose pins are all for other algorithms used to fall through to "unknown" + /// — so somebody in the middle who simply offered a different key type got the first-use question + /// instead of the hard refusal a pin exists to produce. Now a key under an algorithm with no pin of its + /// own is accepted only if it is the same key as one that is pinned (an RSA key is one key under + /// rsa-sha2-256 and rsa-sha2-512, and its fingerprint says so), and is otherwise a + /// mismatch against the endpoint's pin. is what keeps that from + /// refusing a server that still has its pinned key. + /// /// private sealed class HostKeyGate( IKnownHostStore knownHosts, SshConnectionRequest request, + IReadOnlyList endpointPins, IProgress? progress, CancellationToken cancellationToken) { @@ -264,13 +326,21 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts) .GetAwaiter() .GetResult(); - if (pinned is null) + if (pinned is null && endpointPins.Count == 0) { // Refused, not prompted. The caller decides, off the handshake thread. e.CanTrust = false; return; } + // Nothing pinned under this algorithm, but the endpoint has pins: the same key under another + // algorithm's name if any pin's fingerprint is this one, and otherwise a changed key, measured + // against the endpoint's first pin. See the class remarks. + pinned ??= endpointPins + .FirstOrDefault(pin => SshHostKeyFingerprint.Equal(pin.Fingerprint, presentation.Fingerprint)) + ?.Fingerprint + ?? endpointPins[0].Fingerprint; + var matches = SshHostKeyFingerprint.Equal(pinned, presentation.Fingerprint); mismatch = !matches; e.CanTrust = matches; diff --git a/tests/DodoSSH.Client.Ssh.Tests/HostKeyPinningTests.cs b/tests/DodoSSH.Client.Ssh.Tests/HostKeyPinningTests.cs new file mode 100644 index 0000000..c16f237 --- /dev/null +++ b/tests/DodoSSH.Client.Ssh.Tests/HostKeyPinningTests.cs @@ -0,0 +1,69 @@ +namespace DodoSSH.Client.Ssh.Tests; + +/// +/// A pin is a pin for the endpoint, whichever key type the server answers with. +/// +/// +/// Pins are stored per algorithm, and the gap these close was that an endpoint whose only pins were for +/// other algorithms read as a host never seen before. Somebody in the middle needed nothing more than a key +/// of a different type to turn the hard refusal a pin exists for into a first-use question the user is +/// invited to click through. +/// +[Collection(SshCollection.Name)] +public sealed class HostKeyPinningTests(SshServerFixture fixture) +{ + private static CancellationToken Token => TestContext.Current.CancellationToken; + + /// + /// The endpoint is pinned under a key type this server does not have, which is exactly what a server in + /// the middle looks like from here: the pinned key is not on offer, and a key of another type is. + /// + [Fact] + public async Task AKeyOfAnotherTypeOnAPinnedEndpoint_IsAMismatchRatherThanAnUnknownHost() + { + var knownHosts = new InMemoryKnownHostStore(); + + await knownHosts.TrustAsync( + new HostKeyPresentation(fixture.Host, fixture.Port, "ssh-dss", "SHA256:the-key-that-was-approved"), + Token); + + var mismatch = await Should.ThrowAsync(async () => + await new SshNetConnectionFactory(knownHosts).ConnectAsync(Request(), progress: null, Token)); + + mismatch.PinnedFingerprint.ShouldBe("SHA256:the-key-that-was-approved"); + } + + /// + /// The same public key under another algorithm's name — an RSA key signs as rsa-sha2-256 or + /// rsa-sha2-512 — is the key that was approved, and its fingerprint is what says so. + /// + [Fact] + public async Task TheApprovedKeyUnderAnotherAlgorithmName_IsStillTrusted() + { + var learned = new InMemoryKnownHostStore(); + + var unknown = await Should.ThrowAsync(async () => + await new SshNetConnectionFactory(learned).ConnectAsync(Request(), progress: null, Token)); + + var knownHosts = new InMemoryKnownHostStore(); + + await knownHosts.TrustAsync(unknown.Presentation with { Algorithm = "x-renamed-algorithm" }, Token); + + await using var connection = await new SshNetConnectionFactory(knownHosts) + .ConnectAsync(Request(), progress: null, Token); + + connection.ShouldNotBeNull(); + } + + [Fact] + public async Task AnEndpointWithNoPinsAtAll_IsStillAnUnknownHost() => + await Should.ThrowAsync(async () => + await new SshNetConnectionFactory(new InMemoryKnownHostStore()) + .ConnectAsync(Request(), progress: null, Token)); + + private SshConnectionRequest Request() => new( + fixture.Host, + fixture.Port, + SshServerFixture.Username, + new SshPasswordCredential(SshServerFixture.Password)); +}