Public Access
Find out why a connection did not reach, instead of only saying it timed out
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
dabc691c35
commit
fd92b80d4f
6 files changed
+774
-9
No files matched your search
@@ -45,6 +45,31 @@ internal sealed class FakeSshConnectionFactory : ISshConnectionFactory, ISftpSes
|
||||
/// </remarks>
|
||||
internal List<SshConnectionRequest> SftpRequests { get; } = [];
|
||||
|
||||
/// <summary>
|
||||
/// The furthest phase a failing attempt reports before <see cref="Failure"/> is thrown, or null for one
|
||||
/// that never reports any.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// ◆ <b>It is what decides whether the shell goes and probes the endpoint.</b> 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 <c>VaultViewModel.ExplainAsync</c>.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Defaults to <see cref="SshConnectionPhase.Reaching"/>, 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 <c>BuildConnectionInfo</c> — a private key that will not
|
||||
/// parse — which happens before it reports anything.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal SshConnectionPhase? FailAfter { get; set; } = SshConnectionPhase.Reaching;
|
||||
|
||||
/// <summary>Every phase up to and including <paramref name="furthest"/>, in the order they happen.</summary>
|
||||
private static IEnumerable<SshConnectionPhase> PhasesUpTo(SshConnectionPhase? furthest) =>
|
||||
furthest is { } limit
|
||||
? Enum.GetValues<SshConnectionPhase>().Where(phase => phase <= limit)
|
||||
: [];
|
||||
|
||||
/// <inheritdoc />
|
||||
public async Task<ISshConnection> 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();
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A reachability probe that answers from a script instead of from the network.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// ◆ <b>Every shell in this suite is given one, and that is not only about speed.</b> The real probe resolves
|
||||
/// a name and opens a socket — see <see cref="SshReachabilityProbe"/> — so a fixture without this would put a
|
||||
/// live DNS lookup for <c>db.internal</c> 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: <c>db.internal</c>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <see cref="Summary"/> 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal sealed class FakeReachabilityProbe : ISshReachabilityProbe
|
||||
{
|
||||
/// <summary>What every probe comes back with. Empty adds nothing to the failure it explains.</summary>
|
||||
internal string Summary { get; set; } = string.Empty;
|
||||
|
||||
/// <summary>What it was asked about, in order, so a test can assert it was not asked at all.</summary>
|
||||
internal List<string> Probed { get; } = [];
|
||||
|
||||
/// <inheritdoc />
|
||||
public ValueTask<SshReachabilityReport> ProbeAsync(
|
||||
string host,
|
||||
int port,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
Probed.Add($"{host}:{port}");
|
||||
|
||||
return ValueTask.FromResult(
|
||||
new SshReachabilityReport(SshReach.Open, host, port, host, TimeSpan.Zero, Summary));
|
||||
}
|
||||
}
|
||||
@@ -63,6 +63,15 @@ public sealed partial class ShellFlowTests : IAsyncLifetime
|
||||
/// </remarks>
|
||||
private readonly FakeSshConnectionFactory ssh = new();
|
||||
|
||||
/// <summary>
|
||||
/// The scripted answer behind every "why did that not connect" in this file.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Silent by default — see <see cref="FakeReachabilityProbe.Summary"/> — so the cases written before the
|
||||
/// diagnosis existed go on asserting the exact failure message they always did.
|
||||
/// </remarks>
|
||||
private readonly FakeReachabilityProbe reachability = new();
|
||||
|
||||
private int signInAttempts;
|
||||
|
||||
/// <summary>How many times a shell has tried to resume a remembered sign-in, and with what.</summary>
|
||||
@@ -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");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// ◆ <b>The reason this exists: "Connection has timed out." is what SSH.NET says for two different
|
||||
/// faults.</b> 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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[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");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// Past <see cref="SshConnectionPhase.Reaching"/> 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.
|
||||
/// </remarks>
|
||||
[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");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// ◆ <b>The other end of the window, and the one that would state something false.</b> A failure with no
|
||||
/// phase reported at all never dialled — the real factory throws out of <c>BuildConnectionInfo</c> 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.
|
||||
/// </remarks>
|
||||
[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");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// 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,
|
||||
|
||||
@@ -0,0 +1,171 @@
|
||||
using System.Net;
|
||||
using System.Net.Sockets;
|
||||
|
||||
namespace DodoSSH.Client.Ssh.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// The probe that runs after a connection has failed, and the four answers it can come back with.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Real sockets on loopback rather than an abstraction over them, because the thing under test <em>is</em>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class SshReachabilityProbeTests
|
||||
{
|
||||
/// <remarks>
|
||||
/// Short, because two of these cases prove a negative by waiting. The production budget is six seconds —
|
||||
/// see <see cref="SshReachabilityProbe.Budget"/> — 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.
|
||||
/// </remarks>
|
||||
private static readonly TimeSpan ShortBudget = TimeSpan.FromSeconds(2);
|
||||
|
||||
private static CancellationToken Token => TestContext.Current.CancellationToken;
|
||||
|
||||
/// <remarks>
|
||||
/// The case the whole type was written for. SSH.NET reports this and <see cref="NothingListening"/> with
|
||||
/// the identical "Connection has timed out." — so if this comes back as anything but
|
||||
/// <see cref="SshReach.Open"/>, the probe is adding nothing the exception did not already say.
|
||||
/// </remarks>
|
||||
[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");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// A port with nothing on it. The distinction from <see cref="NothingAnswers"/> is the entire value of
|
||||
/// the finding: refused means the machine is up.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ◆ <b>The full budget, and this test is why the production one is six seconds.</b> 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 <see cref="ShortBudget"/> this case comes back
|
||||
/// <see cref="SshReach.NoAnswer"/>, 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[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");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// 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 <see cref="SshReach.NoAnswer"/> instantly — a socket
|
||||
/// erroring for some unrelated reason — would otherwise pass this while telling the user something false
|
||||
/// about their firewall.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[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");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// <c>.invalid</c> 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.
|
||||
/// </remarks>
|
||||
[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");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// 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.
|
||||
/// </remarks>
|
||||
[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)");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// The caller is in a failure path with nothing to do with a second exception — see
|
||||
/// <see cref="ISshReachabilityProbe.ProbeAsync"/> — 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.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task AnEmptyHostIsTheCallersBug_NotAFinding()
|
||||
{
|
||||
await Should.ThrowAsync<ArgumentException>(
|
||||
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;
|
||||
|
||||
/// <summary>A port that was free a moment ago, which is the closest thing to a guarantee there is.</summary>
|
||||
/// <remarks>
|
||||
/// 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.
|
||||
/// </remarks>
|
||||
private static int UnusedPort()
|
||||
{
|
||||
var listener = new TcpListener(IPAddress.Loopback, 0);
|
||||
listener.Start();
|
||||
|
||||
var port = PortOf(listener);
|
||||
listener.Stop();
|
||||
|
||||
return port;
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user