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; + } +}