From fd92b80d4faac98bb6910c9c5faee3a33f7a6f48 Mon Sep 17 00:00:00 2001 From: Laurens Bezemer | DodoTech Date: Wed, 26 Aug 2026 16:04:07 +0200 Subject: [PATCH] Find out why a connection did not reach, instead of only saying it timed out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured against SSH.NET 2026.0.0 before anything was written, because the premise needed checking. Four ways of failing to reach a host, and what this client puts in front of the user for each: server accepts TCP, never sends a banner -> "Connection has timed out." packets black-holed, nothing comes back -> "Connection has timed out." nothing listening on the port -> "...actively refused it." name does not resolve -> "No such host is known." The first two are the same five words and are not the same problem. One is a network, a firewall or a wrong address; the other is a machine that is up and well and an sshd that is wedged, or a port that is not SSH at all. Nothing in the exception tells them apart, and that sentence was the whole of what this product could say about the failure people hit most. ◆ A SOCKET ANSWERS IT, IN ABOUT AS LONG AS IT TAKES TO OPEN ONE. SshReachabilityProbe resolves the name and opens a bare TCP connection, and the four answers it can come back with are the four somebody would do something different about — unresolved, refused, silent, open. Every one of them is what a socket actually did, measured at the moment it was asked, rather than anything inferred from the exception that started it. Deliberately nothing else. No ping: ICMP is filtered on most of the networks this is pointed at, so an unanswered one would be evidence of nothing. No scan of neighbouring ports — the question is what happened to the connection the user asked for, and probing ports nobody asked about is how a client gets flagged by an intrusion detector on the very network somebody is trying to work on. No second SSH handshake against a server that has already failed one. The budget is six seconds and the floor under it is not a guess: a refusal is not immediate on Windows, and a closed loopback port took ~2s to answer when this was measured, because the stack retries the SYN first. At two seconds the probe calls a machine that is up and refusing a firewall. The test for that case runs at the real budget on purpose, so lowering the number fails it. ◆ ONLY WHILE REACHING, AND THE WINDOW IS CLOSED AT BOTH ENDS. Past SshConnectionPhase.Reaching the machine has offered a host key, so it has demonstrably been reached and a refused password is not a reachability question. Before it, nothing has been dialled at all — a private key that will not parse throws out of BuildConnectionInfo before the factory reports anything — and probing there finds a healthy port and appends "the SSH handshake is what did not finish" to a message about an unreadable key. That is a confident wrong answer replacing an accurate one, so both ends are shut. PhaseReporter keeps the furthest phase to decide it. The finding is added to what SSH.NET said rather than replacing it: the exception is the fact, and this is a reading of a second measurement. Every shell in ShellFlowTests now gets a scripted probe. Without one a failed connection in any test in that file would do a live DNS lookup for db.internal and open a real socket — slow, network-dependent, and worse than either, since db.internal resolves on any machine whose resolver has a search domain that makes it, which would make the asserted sentence depend on whose network the suite ran from. ◆ WHAT THIS DOES NOT FIX, FOUND WHILE READING THE SAME PATH. - SshConnectionRequest.ConnectTimeout is documented, plumbed, and passed by nothing. Every connection this client makes is hard-wired to the 15-second default, and there is no per-host field and no setting that can change it. Fifteen seconds bounds the handshake waits as well as the connect — a server that accepts TCP and never sends a banner fails at exactly the timeout, measured — where OpenSSH's own ConnectTimeout covers only the connect. If timeouts are frequent on slow links, this is the first suspect, and it needs a product decision rather than a patch. - The known-host lookup in HostKeyGate.OnHostKeyReceived blocks SSH.NET's message thread inside that same 15-second budget. The blocking is acknowledged in the code; that it competes with the key exchange's own timeout is not. On a cold or locked vault a slow read would surface as a connection timeout, which is exactly the symptom above. - ConnectThroughHostKeyGateAsync never unsubscribes HostKeyReceived on success, so the gate outlives the handshake holding the connect-time CancellationToken. If SSH.NET raises that event again on rekey it would re-enter the blocking store read with a token whose source is long gone. Not verified either way — it needs a real sshd and a forced rekey. Co-Authored-By: Claude Opus 5 --- .../ViewModels/MainWindowViewModel.cs | 19 +- .../ViewModels/VaultViewModel.cs | 147 ++++++++- src/DodoSSH.Client.Ssh/SshReachability.cs | 281 ++++++++++++++++++ tests/DodoSSH.Client.App.Tests/FakeSsh.cs | 71 ++++- .../ShellFlowTests.cs | 94 +++++- .../SshReachabilityProbeTests.cs | 171 +++++++++++ 6 files changed, 774 insertions(+), 9 deletions(-) create mode 100644 src/DodoSSH.Client.Ssh/SshReachability.cs create mode 100644 tests/DodoSSH.Client.Ssh.Tests/SshReachabilityProbeTests.cs diff --git a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs index 8d98741..bc345f1 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs @@ -348,6 +348,18 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp /// private readonly Func>? readClipboard; + /// + /// What explains a connection that would not reach its host, handed to every + /// this shell builds. + /// + /// + /// Held rather than passed straight through, because the vault is not built here: it is built on unlock, + /// which happens once per session and possibly more than once. Null means the standard probe, which is + /// what both heads use — a test passes one to keep a failed connection from putting a real DNS lookup and + /// a real socket in the middle of a unit suite. + /// + private readonly ISshReachabilityProbe? reachability; + /// private readonly Action post; @@ -556,8 +568,10 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp IUpdateChannel? updates = null, Action? post = null, Func? openInEditor = null, - Func? watchFile = null) + Func? watchFile = null, + ISshReachabilityProbe? reachability = null) { + this.reachability = reachability; this.post = post ?? (action => Dispatcher.UIThread.Post(action)); this.paths = paths; @@ -2871,7 +2885,8 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp ReconnectAsync, copyToClipboard, connectionLog, - visibility); + visibility, + reachability); State = ShellState.Unlocked; // Offered only where it can actually be honoured: a machine that can keep a key, and a profile that diff --git a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs index 9ecc870..d75c868 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs @@ -1445,8 +1445,18 @@ internal sealed partial class VaultViewModel( ServerReconnectHandler? reconnect = null, Func? copyToClipboard = null, ConnectionRecorder? connectionLog = null, - VaultVisibility? visibility = null) : ObservableObject, IAsyncDisposable + VaultVisibility? visibility = null, + ISshReachabilityProbe? reachability = null) : ObservableObject, IAsyncDisposable { + /// What explains a failed connection when no caller supplied something else. + /// + /// Static and shared, because holds nothing but a budget and every + /// call to it is a fresh socket. Last in the parameter list and optional for the reason + /// visibility is: every existing caller — which is both heads and every test — goes on meaning + /// exactly what it did, and the one that wants to watch what was probed passes a substitute. + /// + private static readonly ISshReachabilityProbe DefaultReachability = new SshReachabilityProbe(); + /// /// /// A minute. The pull is a delta keyed on a cursor, so an idle pass is one small request and costs the @@ -11233,7 +11243,52 @@ internal sealed partial class VaultViewModel( /// private sealed class PhaseReporter(Action report) : IProgress { - public void Report(SshConnectionPhase value) => report(value); + private int furthest = -1; + + /// + /// The furthest phase reported, or null if the handshake never reported one at all. + /// + /// + /// + /// Kept because a failure cannot be read without it: probes the endpoint + /// only for a connection that stopped while reaching, and this is the record of how far it got. + /// Nothing else asks — the card is driven by the events, not by this. + /// + /// + /// ◆ An behind a pair, because the write and the read are + /// on different threads. Phases are reported from whichever thread the handshake is on — SSH.NET + /// raises host key verification on its own message thread, which is where + /// and + /// come from — and this is read on the UI thread + /// after the attempt has failed. The await in between orders them in practice; nothing in the + /// language says so, and a cached read here would probe a host that had already offered a key. + /// + /// + /// Monotonic, so a phase cannot go backwards. The enum only ever advances today, and the + /// maximum rather than the latest is what keeps that an observation rather than something this type + /// relies on: a rekey mid-handshake raising HostKeyReceived a second time must not be able to + /// walk the record back to from + /// . + /// + /// + public SshConnectionPhase? Furthest => + Volatile.Read(ref furthest) is var seen && seen < 0 ? null : (SshConnectionPhase)seen; + + public void Report(SshConnectionPhase value) + { + var reported = (int)value; + + // Read-compare-write rather than an interlocked maximum, which the BCL has no primitive for. The + // race it loses is two phases reported at the same instant from two threads, which this + // handshake does not do — it is one connection making one call at a time — and the cost of + // losing it would be one phase forgotten rather than a wrong answer. + if (reported > Volatile.Read(ref furthest)) + { + Volatile.Write(ref furthest, reported); + } + + report(value); + } } /// What a manual target reads as, once it has been taken apart. @@ -11506,9 +11561,13 @@ internal sealed partial class VaultViewModel( HostAuthentication authentication, CancellationToken cancellationToken) { + // Built here rather than inside the call, because the catch blocks below read it: how far the + // handshake got is what says whether a failure is worth probing the endpoint over. See ExplainAsync. + var reporter = ReporterFor(attempt); + try { - await ConnectAndAnnounceAsync(attempt, target, authentication, cancellationToken) + await ConnectAndAnnounceAsync(attempt, target, authentication, reporter, cancellationToken) .ConfigureAwait(true); } catch (TimeoutException) @@ -11534,10 +11593,87 @@ internal sealed partial class VaultViewModel( catch (Exception exception) { RecordFailure(target, authentication, ConnectionOutcome.Failed); - Abandon(attempt, exception.Message); + + Abandon( + attempt, + await ExplainAsync(exception, target, reporter, cancellationToken).ConfigureAwait(true)); } } + /// + /// What to tell the user about a connection that failed, having gone and looked when the message alone + /// does not say. + /// + /// + /// + /// ◆ Because "Connection has timed out." is what SSH.NET says for two unrelated faults. A machine + /// that never answered and a machine that answered TCP and then never spoke SSH arrive here as the same + /// five words, and they have nothing in common to do about them — the first is a network, a firewall or a + /// wrong address, the second is an sshd that is wedged or a port that is not SSH. That was the + /// whole of what this screen could say about the failure users hit most, so it now goes and finds out; + /// see for what it does and deliberately does not do. + /// + /// + /// Only for a failure that happened while reaching, and the phase is what settles that. The window + /// is exactly , and it is closed at both ends. Past it the + /// machine has demonstrably been reached — it offered a host key — so a refused password or a key the + /// remote would not take is not a reachability question, and probing the port would answer one nobody + /// asked while telling somebody their network is fine when they already know. + /// + /// + /// ◆ And before it, nothing has been dialled yet, which is the half that would lie. No phase at + /// all means the attempt never reached ConnectAsync: a renderer that did not attach, a credential + /// that would not decrypt, a private key that would not parse — BuildConnectionInfo throws on that + /// last one before the factory reports anything. Probing there finds a healthy port and appends "the SSH + /// handshake is what did not finish" to a message about an unreadable key, which is a confident wrong + /// answer where there was an accurate one. The phases are reported at the instant each begins and are not + /// guesses; see the enum. + /// + /// + /// The original message is kept and the finding is added to it. What SSH.NET said is the fact, and + /// this is a reading of a second measurement — dropping the first would leave nothing to check the second + /// against, and would lose the one case where the exception is already specific. + /// + /// + /// A probe that itself fails changes nothing. The user is being told about a connection that did + /// not happen; a diagnostic that cannot run is not a second failure worth reporting, and must not be able + /// to replace the first with its own exception. Cancellation is in that: a user who closed the tab while + /// this ran is not waiting for the answer. + /// + /// + private async Task ExplainAsync( + Exception exception, + ConnectionTarget target, + PhaseReporter reporter, + CancellationToken cancellationToken) + { + if (reporter.Furthest != SshConnectionPhase.Reaching) + { + return exception.Message; + } + + try + { + var report = await Reachability + .ProbeAsync(target.Hostname, target.Port, cancellationToken) + .ConfigureAwait(true); + + // A probe with nothing to say leaves the message exactly as it was, rather than appending a + // space to it. The real one always has a sentence; this is what keeps a substitute that does not + // from changing the text of every failure that goes past it. + return report.Summary is { Length: > 0 } finding + ? $"{exception.Message} {finding}" + : exception.Message; + } + catch (Exception probe) when (probe is not OutOfMemoryException) + { + return exception.Message; + } + } + + /// + private ISshReachabilityProbe Reachability => reachability ?? DefaultReachability; + /// What a renderer that never attached is reported as. /// /// @@ -11603,6 +11739,7 @@ internal sealed partial class VaultViewModel( ConnectionAttemptEventArgs attempt, ConnectionTarget target, HostAuthentication authentication, + PhaseReporter reporter, CancellationToken cancellationToken) { // Not reported before the await: the tab is constructed with this step already running — see @@ -11616,7 +11753,7 @@ internal sealed partial class VaultViewModel( authentication.Credential); var sessionId = await workspace - .OpenSessionAsync(request, TerminalSize.Default, ReporterFor(attempt), cancellationToken) + .OpenSessionAsync(request, TerminalSize.Default, reporter, cancellationToken) .ConfigureAwait(true); // The workspace has already opened a ticket for this session, with the address and the moment it diff --git a/src/DodoSSH.Client.Ssh/SshReachability.cs b/src/DodoSSH.Client.Ssh/SshReachability.cs new file mode 100644 index 0000000..57df85e --- /dev/null +++ b/src/DodoSSH.Client.Ssh/SshReachability.cs @@ -0,0 +1,281 @@ +using System.Diagnostics; +using System.Globalization; +using System.Net; +using System.Net.Sockets; + +namespace DodoSSH.Client.Ssh; + +/// +/// What one look at an endpoint found, once a connection to it had already failed. +/// +/// +/// +/// ◆ Four, because they are the four a user would do something different about. A name that does not +/// resolve is a typo or a VPN that is down; a port that refuses is a machine that is up with no sshd +/// on it; a port that says nothing at all is a firewall or a machine that is off; and a port that opens is +/// the one that matters most, because it means the network is fine and the SSH handshake is what failed. +/// +/// +/// Nothing here is inferred from the exception that started it. Each value is what a socket actually +/// did, measured at the moment it was asked, which is what makes it safe for a screen to state as fact — +/// the same rule is written to. +/// +/// +public enum SshReach +{ + /// The name does not resolve on this machine, so there was no address to dial. + Unresolved = 0, + + /// It resolved, and the port sent nothing back before the probe gave up. + NoAnswer = 1, + + /// It resolved, and the host said at once that nothing is listening there. + Refused = 2, + + /// It resolved, and something accepted a plain TCP connection on that port. + Open = 3, +} + +/// What a probe of one endpoint found, and the sentence that says so. +/// Which of the four it was. +/// The name or address that was probed, as the connection named it. +/// The port that was probed. +/// +/// The address the name resolved to and the probe actually dialled, or null when it did not resolve. +/// +/// One rather than all of them. A name with several addresses is dialled at whichever the stack picks +/// first, and naming the one that was tried is a fact; listing the rest would suggest they were. +/// +/// +/// How long the probe took, which is the evidence behind . +/// The finding in plain words, ready to put in front of somebody. +public sealed record SshReachabilityReport( + SshReach Reach, + string Host, + int Port, + string? Address, + TimeSpan Elapsed, + string Summary); + +/// Looks at an endpoint that would not connect, and reports what is actually wrong with it. +/// +/// Separate from rather than a method on it, because it is not a way of +/// connecting: it runs after one has failed, it authenticates nothing, and it must stay usable from a caller +/// that has no credential to hand — which the failure path is, by the time it needs this. +/// +public interface ISshReachabilityProbe +{ + /// + /// Resolves the name and opens a bare TCP connection to the port, to find out which part is broken. + /// + /// The hostname or address the failed connection named. + /// The port it named. + /// Abandons the probe. + /// + /// Never throws for a host that is simply broken — being unreachable is the answer, not an error. A + /// caller is already in a failure path and has nothing useful to do with a second exception. + /// + ValueTask ProbeAsync(string host, int port, CancellationToken cancellationToken); +} + +/// +/// +/// +/// ◆ Why this exists at all: two unrelated failures arrive as the same sentence. SSH.NET reports both +/// "the machine never answered" and "the machine answered TCP and then never spoke SSH" as +/// SshOperationTimeoutException with the message "Connection has timed out." Those are not the same +/// problem and do not have the same fix — one is a network or a firewall, the other is an sshd that +/// is wedged or a port that is not SSH at all — and nothing in the exception tells them apart. A socket does, +/// in about as long as it takes to open one. +/// +/// +/// A plain TCP connect and a name lookup, and deliberately nothing else. No ping, because ICMP is +/// filtered on most of the networks this product is pointed at and an unanswered ping would be evidence of +/// nothing. No scan of neighbouring ports: the question is what happened to the connection the user asked +/// for, and probing ports nobody asked about is the kind of thing that gets a client flagged by an intrusion +/// detector on the very network somebody is trying to work on. No SSH banner read either — that is a second +/// handshake against a server that has already failed one, and the open socket has answered the question. +/// +/// +/// The whole probe is bounded, and the bound is short. By the time this runs the user has already +/// waited out a connect timeout, so the budget here is a few seconds spent buying an answer rather than +/// another wait to sit through. Exceeding it is itself a finding — nothing came back — so the deadline +/// produces a result rather than an error. +/// +/// +public sealed class SshReachabilityProbe : ISshReachabilityProbe +{ + /// + /// How long the name lookup and the connection attempt get between them. + /// + /// + /// + /// A budget rather than two timeouts, so a slow resolver cannot spend the socket's share and leave the + /// interesting half of the probe unrun. It is well under the fifteen this client allows a real connection + /// — see SshNetConnectionFactory — because a machine that answers at all answers a bare TCP + /// connect in a fraction of what an SSH handshake takes, and everything past that point is waiting to + /// confirm silence somebody has already waited out once. + /// + /// + /// ◆ Six rather than two, and the floor under it is not a guess. A refusal is not instant on + /// Windows: a closed port on loopback took ~2s to come back as WSAECONNREFUSED when this was + /// measured, because the stack retries the SYN before reporting. Anything at or under that turns + /// — a machine that is up — into , which + /// this client would then explain as a firewall. The budget must clear the platform's SYN-retry time + /// with room to spare, and a couple of seconds of headroom is what the extra four buy. Shortening this + /// without re-measuring that floor reintroduces a wrong answer rather than a slow one. + /// + /// + public static readonly TimeSpan Budget = TimeSpan.FromSeconds(6); + + private readonly TimeSpan budget; + + /// Uses the standard . + public SshReachabilityProbe() + : this(Budget) + { + } + + /// + /// How long the whole probe gets. A test uses this to keep a deliberately silent endpoint from costing + /// the suite six seconds; nothing in the application passes one. + /// + public SshReachabilityProbe(TimeSpan budget) => this.budget = budget; + + /// + public async ValueTask ProbeAsync( + string host, + int port, + CancellationToken cancellationToken) + { + ArgumentException.ThrowIfNullOrWhiteSpace(host); + + var clock = Stopwatch.StartNew(); + + using var deadline = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + deadline.CancelAfter(budget); + + var address = await ResolveAsync(host, deadline.Token).ConfigureAwait(false); + + if (address is null) + { + return Report(SshReach.Unresolved, host, port, address: null, clock.Elapsed); + } + + var reach = await ConnectAsync(address, port, deadline.Token).ConfigureAwait(false); + + return Report(reach, host, port, address.ToString(), clock.Elapsed); + } + + /// + /// The address the connection would have been made to, or null if the name does not resolve. + /// + /// + /// An address typed in rather than a name costs no lookup: + /// settles it here, where would go and ask the resolver about a string it is going to + /// hand straight back. That matters for the report as much as for the speed — a literal that "resolved" + /// would be a fact this probe invented about a machine it never asked anybody about. + /// + private static async ValueTask ResolveAsync(string host, CancellationToken cancellationToken) + { + if (IPAddress.TryParse(host, out var literal)) + { + return literal; + } + + try + { + var addresses = await Dns.GetHostAddressesAsync(host, cancellationToken).ConfigureAwait(false); + + // First rather than a preference between v4 and v6: this is reporting on the connection that + // failed, and the stack picked from this same list in this same order when it made it. + return addresses.Length == 0 ? null : addresses[0]; + } + catch (Exception exception) when (exception is SocketException or OperationCanceledException) + { + // A resolver that refuses and a resolver that never answers are the same finding here: there is + // no address, so there was nothing to dial. Which of the two it was is a DNS question, and this + // probe would be guessing at the answer. + return null; + } + } + + /// + /// The socket is opened and dropped without a byte being sent. A TCP handshake is the whole question — + /// whether anything is listening — and writing to a server that is about to be told nothing is wanted + /// would leave a connection attempt in its log that the user did not make. + /// + private static async ValueTask ConnectAsync( + IPAddress address, + int port, + CancellationToken cancellationToken) + { + using var socket = new Socket(address.AddressFamily, SocketType.Stream, ProtocolType.Tcp); + + try + { + await socket.ConnectAsync(new IPEndPoint(address, port), cancellationToken).ConfigureAwait(false); + + return SshReach.Open; + } + catch (SocketException exception) when (exception.SocketErrorCode is SocketError.ConnectionRefused) + { + return SshReach.Refused; + } + catch (Exception exception) when (exception is SocketException or OperationCanceledException) + { + // Everything else the stack can say here — unreachable, timed out, the deadline above — comes to + // the same account of it: the port never answered. Naming the OS error instead would be trading a + // sentence somebody can act on for one they would have to look up. + return SshReach.NoAnswer; + } + } + + private static SshReachabilityReport Report( + SshReach reach, + string host, + int port, + string? address, + TimeSpan elapsed) + => new(reach, host, port, address, elapsed, Describe(reach, host, port, address, elapsed)); + + /// + /// The finding as a sentence, saying what was measured and then what it means. + /// + /// + /// Measurement first in every case, on the rule the rest of this product's failure text follows: the + /// fact is what the user can check and act on, and the interpretation after it is this client's reading + /// of that fact. Each reading is hedged exactly as far as it deserves — a refusal has one common cause + /// and names it, silence has three and lists them, and an open port names the class of problem rather + /// than pretending to know which member of it this is. + /// + private static string Describe(SshReach reach, string host, int port, string? address, TimeSpan elapsed) + { + var seconds = elapsed.TotalSeconds.ToString("0.#", CultureInfo.CurrentCulture); + var where = address is null || string.Equals(address, host, StringComparison.OrdinalIgnoreCase) + ? host + : $"{host} ({address})"; + + return reach switch + { + SshReach.Unresolved => + $"The name {host} does not resolve on this machine, so nothing was ever dialled. Check the " + + "spelling, or whether the VPN or DNS server this name depends on is up.", + + SshReach.Refused => + $"{where} is reachable and refused port {port} immediately: the machine is up and nothing is " + + "listening there. Either sshd is not running, or this is the wrong port.", + + SshReach.NoAnswer => + $"{where} sent nothing back on port {port} within {seconds}s. A machine that was up and had " + + "nothing listening would have refused at once, so this is a firewall dropping the " + + "packets, a machine that is off, or the wrong address.", + + _ => + $"Port {port} on {where} accepts connections, so the network and the machine are fine and " + + "something is listening. The SSH handshake is what did not finish: an sshd that is " + + "overloaded or wedged, a middlebox holding the connection open, or a service on this " + + "port that is not SSH.", + }; + } +} diff --git a/tests/DodoSSH.Client.App.Tests/FakeSsh.cs b/tests/DodoSSH.Client.App.Tests/FakeSsh.cs index 50d7620..355e631 100644 --- a/tests/DodoSSH.Client.App.Tests/FakeSsh.cs +++ b/tests/DodoSSH.Client.App.Tests/FakeSsh.cs @@ -45,6 +45,31 @@ internal sealed class FakeSshConnectionFactory : ISshConnectionFactory, ISftpSes /// internal List SftpRequests { get; } = []; + /// + /// The furthest phase a failing attempt reports before is thrown, or null for one + /// that never reports any. + /// + /// + /// + /// ◆ It is what decides whether the shell goes and probes the endpoint. A connection that stopped + /// while reaching is worth asking a socket about; one that got a host key has plainly reached the machine, + /// and one that never reported a phase never dialled at all. See VaultViewModel.ExplainAsync. + /// + /// + /// Defaults to , which is what this fake reported + /// unconditionally before the field existed, so every test written against it goes on meaning what it did. + /// Null models the real factory throwing out of BuildConnectionInfo — a private key that will not + /// parse — which happens before it reports anything. + /// + /// + internal SshConnectionPhase? FailAfter { get; set; } = SshConnectionPhase.Reaching; + + /// Every phase up to and including , in the order they happen. + private static IEnumerable PhasesUpTo(SshConnectionPhase? furthest) => + furthest is { } limit + ? Enum.GetValues().Where(phase => phase <= limit) + : []; + /// public async Task ConnectAsync( SshConnectionRequest request, @@ -56,7 +81,12 @@ internal sealed class FakeSshConnectionFactory : ISshConnectionFactory, ISftpSes // Before the gate rather than after it, which is what makes this fake useful for the connecting // card: a test that holds Gate open is a connection stuck partway through, and the step list has to // show it stuck on a named step rather than on none. - progress?.Report(SshConnectionPhase.Reaching); + // + // How far a failing attempt gets is FailAfter's to say; a succeeding one is always reaching by now. + foreach (var phase in Failure is null ? [SshConnectionPhase.Reaching] : PhasesUpTo(FailAfter)) + { + progress?.Report(phase); + } if (Gate is { } gate) { @@ -419,3 +449,42 @@ internal sealed class FakeSshShellSession : ISshShellSession closed.Dispose(); } } + +/// +/// A reachability probe that answers from a script instead of from the network. +/// +/// +/// +/// ◆ Every shell in this suite is given one, and that is not only about speed. The real probe resolves +/// a name and opens a socket — see — so a fixture without this would put a +/// live DNS lookup for db.internal and a live TCP connect in the middle of a unit suite, on every test +/// where a connection fails. That is slow, it needs a network, and it is worse than either: db.internal +/// resolves on any machine whose resolver has a search domain that makes it resolve, so the sentence the +/// assertions read would depend on whose network the suite was run from. +/// +/// +/// defaults to empty, which is what keeps the tests that predate the probe asserting +/// the exact message they always did: an empty finding appends nothing, so those cases go on describing a +/// failure and nothing else. A test that is about the diagnosis sets it. +/// +/// +internal sealed class FakeReachabilityProbe : ISshReachabilityProbe +{ + /// What every probe comes back with. Empty adds nothing to the failure it explains. + internal string Summary { get; set; } = string.Empty; + + /// What it was asked about, in order, so a test can assert it was not asked at all. + internal List Probed { get; } = []; + + /// + public ValueTask ProbeAsync( + string host, + int port, + CancellationToken cancellationToken) + { + Probed.Add($"{host}:{port}"); + + return ValueTask.FromResult( + new SshReachabilityReport(SshReach.Open, host, port, host, TimeSpan.Zero, Summary)); + } +} diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index a39f59d..2973a77 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -63,6 +63,15 @@ public sealed partial class ShellFlowTests : IAsyncLifetime /// private readonly FakeSshConnectionFactory ssh = new(); + /// + /// The scripted answer behind every "why did that not connect" in this file. + /// + /// + /// Silent by default — see — so the cases written before the + /// diagnosis existed go on asserting the exact failure message they always did. + /// + private readonly FakeReachabilityProbe reachability = new(); + private int signInAttempts; /// How many times a shell has tried to resume a remembered sign-in, and with what. @@ -212,7 +221,12 @@ public sealed partial class ShellFlowTests : IAsyncLifetime { save = onChanged; return new StopWatching(() => save = null); - }); + }, + + // Scripted rather than real, so that a failed connection in any test in this file does not go + // and do a DNS lookup and open a socket. See FakeReachabilityProbe for why that matters beyond + // the wall clock. + reachability: reachability); return ValueTask.CompletedTask; } @@ -2516,6 +2530,84 @@ public sealed partial class ShellFlowTests : IAsyncLifetime tab.Steps[1].IsRunning.ShouldBeTrue("nothing moved it on after the shell let go of it"); } + /// + /// + /// ◆ The reason this exists: "Connection has timed out." is what SSH.NET says for two different + /// faults. A machine that never answered and a machine that answered TCP and then never spoke SSH are + /// the same five words and nothing else, and they are not the same problem. So a failure that stopped + /// while reaching now goes and looks, and what it finds is added to what was said. + /// + /// + /// The finding is appended rather than substituted, which is the half worth asserting: the exception is + /// the fact and the probe is a second measurement, and a test that only checked for the new sentence + /// would pass against an implementation that had thrown the original away. + /// + /// + [Fact] + public async Task AConnectionThatCouldNotReachItsHost_SaysWhatIsActuallyWrongWithIt() + { + var vault = await ReadyToConnectAsync(); + + await using var renderer = await FakeRenderer.AttachAsync(workspace, Token); + + ssh.Failure = new InvalidOperationException("Connection has timed out."); + reachability.Summary = "Port 22 on db.internal accepts connections."; + + await vault.ConnectCommand.ExecuteAsync(null); + + var tab = shell.Tabs.ShouldHaveSingleItem(); + + tab.Status.ShouldBe("Connection has timed out. Port 22 on db.internal accepts connections."); + reachability.Probed.ShouldHaveSingleItem().ShouldBe("db.internal:22"); + } + + /// + /// Past the machine has offered a host key, so it is plainly + /// reachable and there is nothing a socket could add. A refused password that came back with "the network + /// is fine and something is listening" bolted on would be true, useless, and read as though the client + /// had not understood the question. + /// + [Fact] + public async Task AConnectionRefusedAfterTheHostKey_IsNotProbed() + { + var vault = await ReadyToConnectAsync(); + + await using var renderer = await FakeRenderer.AttachAsync(workspace, Token); + + ssh.FailAfter = SshConnectionPhase.Authenticating; + ssh.Failure = new InvalidOperationException("Permission denied (publickey,password)."); + reachability.Summary = "this must not be appended"; + + await vault.ConnectCommand.ExecuteAsync(null); + + shell.Tabs.ShouldHaveSingleItem().Status.ShouldBe("Permission denied (publickey,password)."); + reachability.Probed.ShouldBeEmpty("the host key proved the machine was reached"); + } + + /// + /// ◆ The other end of the window, and the one that would state something false. A failure with no + /// phase reported at all never dialled — the real factory throws out of BuildConnectionInfo on a + /// private key it cannot parse, before the socket is opened. Probing there finds a perfectly healthy port + /// and appends "the SSH handshake is what did not finish" to a message about an unreadable key, which is + /// a confident wrong answer replacing an accurate one. + /// + [Fact] + public async Task AFailureBeforeAnythingWasDialled_IsNotProbed() + { + var vault = await ReadyToConnectAsync(); + + await using var renderer = await FakeRenderer.AttachAsync(workspace, Token); + + ssh.FailAfter = null; + ssh.Failure = new InvalidOperationException("The private key could not be read."); + reachability.Summary = "this must not be appended"; + + await vault.ConnectCommand.ExecuteAsync(null); + + shell.Tabs.ShouldHaveSingleItem().Status.ShouldBe("The private key could not be read."); + reachability.Probed.ShouldBeEmpty("nothing had been dialled, so there was nothing to ask about"); + } + /// /// A refusal has to end up somewhere the user will see it, and by the time one arrives they are quite /// likely looking at another screen — which is exactly what not blocking bought. The tab is that place, diff --git a/tests/DodoSSH.Client.Ssh.Tests/SshReachabilityProbeTests.cs b/tests/DodoSSH.Client.Ssh.Tests/SshReachabilityProbeTests.cs new file mode 100644 index 0000000..ea63ccf --- /dev/null +++ b/tests/DodoSSH.Client.Ssh.Tests/SshReachabilityProbeTests.cs @@ -0,0 +1,171 @@ +using System.Net; +using System.Net.Sockets; + +namespace DodoSSH.Client.Ssh.Tests; + +/// +/// The probe that runs after a connection has failed, and the four answers it can come back with. +/// +/// +/// +/// Real sockets on loopback rather than an abstraction over them, because the thing under test is +/// what a socket does: the whole reason this type exists is that a refusal and a silence are +/// indistinguishable in the exception SSH.NET throws and completely distinguishable to a TCP connect. A fake +/// socket would be this suite asserting that the mapping table maps, and would have passed just as happily +/// against the bug that prompted the work. +/// +/// +/// No container and no sshd. Nothing here speaks SSH — the probe deliberately does not — so the fixture is a +/// listener that accepts, a port nothing is on, and a name that does not resolve. +/// +/// +public sealed class SshReachabilityProbeTests +{ + /// + /// Short, because two of these cases prove a negative by waiting. The production budget is six seconds — + /// see — and nothing about the branch under test changes with + /// the number, so the suite buys the same coverage for a fraction of the wall clock. + /// + private static readonly TimeSpan ShortBudget = TimeSpan.FromSeconds(2); + + private static CancellationToken Token => TestContext.Current.CancellationToken; + + /// + /// The case the whole type was written for. SSH.NET reports this and with + /// the identical "Connection has timed out." — so if this comes back as anything but + /// , the probe is adding nothing the exception did not already say. + /// + [Fact] + public async Task APortThatAcceptsIsOpen_WhichIsWhatTellsAWedgedSshdFromABrokenNetwork() + { + using var listener = Accepting(); + + var report = await new SshReachabilityProbe(ShortBudget) + .ProbeAsync("127.0.0.1", PortOf(listener), Token); + + report.Reach.ShouldBe(SshReach.Open); + report.Address.ShouldBe("127.0.0.1"); + report.Summary.ShouldContain("handshake"); + + // The half a user acts on: nothing here may suggest the network is at fault, because it demonstrably + // is not — a socket just opened over it. + report.Summary.ShouldNotContain("firewall"); + } + + /// + /// + /// A port with nothing on it. The distinction from is the entire value of + /// the finding: refused means the machine is up. + /// + /// + /// ◆ The full budget, and this test is why the production one is six seconds. A refusal is not + /// immediate on Windows — the stack retries the SYN, and a closed loopback port took ~2s to answer when + /// this was written. Run against this case comes back + /// , which is the probe telling somebody a machine that is up and + /// refusing is behind a firewall. Left at the real budget on purpose: it is the one case whose + /// correctness depends on the number, so it is the one that should fail if the number is lowered. + /// + /// + [Fact] + public async Task NothingListening() + { + var report = await new SshReachabilityProbe(SshReachabilityProbe.Budget) + .ProbeAsync("127.0.0.1", UnusedPort(), Token); + + report.Reach.ShouldBe(SshReach.Refused); + report.Summary.ShouldContain("sshd is not running"); + } + + /// + /// + /// 192.0.2.1 is TEST-NET-1 from RFC 5737: reserved for documentation, routed nowhere, and guaranteed not + /// to be some real machine that answers and makes this flap. The packets go out and nothing comes back, + /// which is exactly the shape of a firewall that drops rather than rejects. + /// + /// + /// The elapsed assertion is the one that matters beside the enum. Silence is only evidence if the probe + /// actually waited for it, and a bug that returned instantly — a socket + /// erroring for some unrelated reason — would otherwise pass this while telling the user something false + /// about their firewall. + /// + /// + [Fact] + public async Task NothingAnswers() + { + var report = await new SshReachabilityProbe(ShortBudget).ProbeAsync("192.0.2.1", 22, Token); + + report.Reach.ShouldBe(SshReach.NoAnswer); + report.Elapsed.ShouldBeGreaterThan(TimeSpan.FromSeconds(1)); + report.Summary.ShouldContain("firewall"); + } + + /// + /// .invalid is reserved by RFC 2606 precisely so that a test may rely on it never resolving, on + /// any network, including one whose resolver answers every NXDOMAIN with a search page. + /// + [Fact] + public async Task ANameThatDoesNotResolve() + { + var report = await new SshReachabilityProbe(ShortBudget) + .ProbeAsync("dodossh-no-such-host.invalid", 22, Token); + + report.Reach.ShouldBe(SshReach.Unresolved); + report.Address.ShouldBeNull("there is no address, and inventing one would be the whole failure"); + report.Summary.ShouldContain("does not resolve"); + } + + /// + /// An address typed in is not looked up, and the report must not read as though it had been. The + /// assertion is on the sentence rather than on a call count: what would be wrong is the user being shown + /// "127.0.0.1 (127.0.0.1)", which is what a probe that resolved a literal and then printed both produces. + /// + [Fact] + public async Task AnAddressIsNotPrintedTwice() + { + using var listener = Accepting(); + + var report = await new SshReachabilityProbe(ShortBudget) + .ProbeAsync("127.0.0.1", PortOf(listener), Token); + + report.Summary.ShouldNotContain("(127.0.0.1)"); + } + + /// + /// The caller is in a failure path with nothing to do with a second exception — see + /// — so every branch above returns a report. This is the + /// one input that is a caller's mistake rather than a broken host, and it is the one that still throws. + /// + [Fact] + public async Task AnEmptyHostIsTheCallersBug_NotAFinding() + { + await Should.ThrowAsync( + async () => await new SshReachabilityProbe(ShortBudget).ProbeAsync(" ", 22, Token)); + } + + private static TcpListener Accepting() + { + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + + return listener; + } + + private static int PortOf(TcpListener listener) => ((IPEndPoint)listener.LocalEndpoint).Port; + + /// A port that was free a moment ago, which is the closest thing to a guarantee there is. + /// + /// Bound and released rather than picked out of the air: the OS hands out a port it knows is unused, and + /// the window in which something else could take it is far smaller than the odds of a hard-coded number + /// colliding with whatever the machine running this happens to have open. + /// + private static int UnusedPort() + { + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + + var port = PortOf(listener); + listener.Stop(); + + return port; + } +}