diff --git a/docs/reaching-a-host-you-cannot-dial.md b/docs/reaching-a-host-you-cannot-dial.md index 3538451..92e0a5c 100644 --- a/docs/reaching-a-host-you-cannot-dial.md +++ b/docs/reaching-a-host-you-cannot-dial.md @@ -17,10 +17,10 @@ jump chain — see the last section for why that was wrong. > | Step | State | Notes | > | --- | --- | --- | > | 0. Stop promising the relay | **Done** | Both heads say the box is not wired up and that ticking it stores the address and changes nothing. Left tickable, so a host already carrying the flag can lose it | -> | 1. The loopback bridge | Not started | ADR 0004's "one mechanism, two features" | -> | 2. Jump hosts over it | Not started | No server change at all | -> | 3. The relay over it | Not started | Ticket call, WebSocket, then the same bridge | -> | 4. File transfer parity | Not started | The transfers screen opens its own connection | +> | 1. Dial through a loopback proxy | **Done** | `SshLoopbackProxy` on the request, honoured in `BuildConnectionInfo`. **Not the bridge** — see below | +> | 2. Jump hosts over it | Not started | No server change, and no bridge either: SSH.NET's own forward is the proxy | +> | 3. The relay: the bridge, then the ticket | Not started | The bridge's only consumer, so it lands with the feature that uses it | +> | 4. File transfer parity | Not started | The transfers screen opens its own connection. Already half-done — `OpenSftpAsync` shares `BuildConnectionInfo`, so it honours a proxy today; what is missing is anything passing it one | ## ◆ The relay's checkbox is a false promise, and that is a defect @@ -98,26 +98,56 @@ AuthenticationMethod[])` with `ProxyTypes.Socks5` — checked in `Renci.SshNet.x - **Relay:** the same shape with a different thing on the loopback socket — a listener that pipes bytes into the `dodossh.relay.v1` WebSocket instead of into a bastion's forward. -Which means the transport work is shared and the ordering is: bridge, then the cheap feature, then the one -that needs the server. +### ◆ But the bridge is the relay's half, not the shared one + +Written into step 1 of this plan and wrong. Building it turned the sentence around: **the jump-host path +needs no bridge from this repository at all.** `ForwardedPortDynamic` *is* the loopback listener — SSH.NET +accepts on it, speaks SOCKS5 on it, and tunnels what it accepts through the bastion. Nothing is left for a +bridge of ours to do. The relay is the case with no `SshClient` to hang a forward off, so it is the one that +needs a listener written here, and it is the bridge's only consumer. + +What the two genuinely share is one step lower: **the connection being told to reach its target through a +loopback SOCKS5 proxy, while staying about the target**. That is the piece both features stand on, it is +fifteen lines in `BuildConnectionInfo`, and it is what step 1 turned out to be. + +So the bridge moves to step 3 and lands with the feature that uses it. Building it now would have been a +component whose only caller was two steps away — which is the shape of the two defects this document is +about. ## The work, in order -**0. Stop promising the relay.** The checkbox states that the relay is not wired up yet. One line on each -head, and it is the only step that should ship on its own. +**0. Stop promising the relay.** ✅ The checkbox states that the relay is not wired up yet. One line on each +head, and the only step that should ship on its own. -**1. The bridge.** A loopback `TcpListener` on an ephemeral port that accepts exactly one connection, hands -it to a `Stream` supplied by whoever opened the bridge, and disposes with the session. It belongs in -`Client.Ssh` beside `SshNetConnectionFactory`, and it needs to bind `127.0.0.1` explicitly — a bridge on -`0.0.0.0` is an open SOCKS proxy on the user's network for the life of a shell. +**1. Dial through a loopback proxy.** ✅ `SshConnectionRequest` carries an optional `SshLoopbackProxy`, and +`BuildConnectionInfo` builds SSH.NET's proxy `ConnectionInfo` when it is there. Three properties are worth +knowing, and each is held by a test in `LoopbackProxyTests`: -**2. Jump hosts.** No server change. In order: +- **The type is a port and nothing else.** A proxy on any interface but loopback cannot be expressed, which + matters because the failure mode is an open SOCKS proxy on the user's network for the life of a shell, + and nothing would report it. +- **SOCKS5 rather than a dumb pipe, so the target stays the target.** The host and port in the request are + the ones SSH.NET dials *through* the proxy and the ones the host key gate pins — so the same machine + reached through a bastion is pinned under its own name, not under `127.0.0.1:`, which is not + an identity at all. +- **A proxy that is not listening fails as a connection error**, not as an unknown host key. The gate + translates "no host key seen" into a fingerprint prompt, and a connection that never reached a server has + seen none either — so the prompt would offer to fix the wrong thing, with no fingerprint to show. -- `SshConnectionRequest` grows a route: the resolved chain, each hop carrying what a connect needs, so the - SSH layer is handed hops rather than ids and never looks anything up. +Nothing calls it with a proxy yet. That is deliberate and it is one step wide: step 2 is the caller. + +**2. Jump hosts.** No server change, and no bridge. In order: + +- The hop's own connection, and a `ForwardedPortDynamic("127.0.0.1", 0)` started on it, whose `BoundPort` + becomes the `SshLoopbackProxy` for the connection after it. A chain of two is that twice. - `VaultViewModel` resolves `JumpHostIds` to hosts in the same vault, applying group inheritance per hop the way the target already gets it, and refuses a chain that crosses a vault — the same refusal `RefusesTheDrop` and the group picker already make, for the same reason. +- ◆ **The target's address is resolved at the last hop, not here.** A SOCKS CONNECT names the target and the + bastion resolves it, so what has to be stored on the host is the address *the bastion* can reach — which + is what an `ssh_config` means by `HostName` beside a `ProxyJump`, and what the importer is already + carrying across verbatim. Nothing needs to change for that to be true; it needs to be said, because a + host that resolves here and not there fails as a SOCKS "general failure" naming neither end. - Per-hop host keys. Each hop is a separate handshake against a separate endpoint, so the pin, the unknown key prompt and the changed-key refusal run per hop. **The prompt has to name which hop it is about**, or somebody approves a bastion's fingerprint believing it is the target's — see `HostKeyCard`, which is built @@ -131,9 +161,13 @@ it to a `Stream` supplied by whoever opened the bridge, and disposes with the se - The editor: a picker over other hosts in the same vault, and the host detail's subtitle finally getting the `⤷ bastion-eu` the design asked for. -**3. The relay.** `POST /relay/tickets` with the host id, then the WebSocket with the ticket in -`Sec-WebSocket-Protocol`, piped into the bridge from step 1. The ticket is single-use and expires in 30 -seconds, so it is fetched per connect and never cached. Then the checkbox from step 0 becomes true. +**3. The relay, and the bridge with it.** A loopback `TcpListener` on an ephemeral port that accepts one +connection, answers a SOCKS5 CONNECT on it, and pipes the rest into the `dodossh.relay.v1` WebSocket — +SOCKS5 rather than a raw pipe so that this path presents the same interface step 1 already speaks, and the +target's identity stays the target's. Then `POST /relay/tickets` with the host id and the WebSocket with the +ticket in `Sec-WebSocket-Protocol`; the ticket is single-use and expires in 30 seconds, so it is fetched per +connect and never cached. The bridge binds `127.0.0.1` explicitly, accepts once and stops listening. Then +the sentence step 0 added comes out of both heads. **4. File transfer.** `ISftpSessionFactory.OpenSftpAsync` opens its own second connection, so a host that needs a chain or a relay to reach needs it there too, or SFTP silently fails for exactly the hosts this @@ -154,9 +188,17 @@ discovering it as a stack overflow inside a connect. through `HostInheritance` exactly as the target does. Skipping that dials 22 as nobody on a bastion that is on 2222 as `deploy`. -**`ForwardedPortDynamic(0)` and reading the port back.** Binding an ephemeral port and then asking for the -one that was assigned is the part that varies between SSH.NET versions; pin it with a test that opens one -against the test `sshd` rather than trusting the number. +**`ForwardedPortDynamic(0)` reports its port, and the one-argument constructor's bind address is +undocumented.** The first is now pinned by a test rather than trusted. The second is why the two-argument +constructor is always used: if the default were `0.0.0.0` the failure would not be a test failure, it would +be a SOCKS proxy into the developer's network that nothing reports, so the bound host is asserted too. + +**◆ The test server refuses forwarding, and says so nowhere useful.** `linuxserver/openssh-server` ships +`AllowTcpForwarding no`. A dynamic forward starts anyway — opening one asks the server nothing — and every +connection through it is then refused at channel-open, which SSH.NET surfaces as `SOCKS5: General failure`, +naming neither the server nor the setting. `SshServerFixture` patches it after start and HUPs sshd. **There +are two `sshd_config` files in that image** and the running server uses `/config/sshd/sshd_config`; patching +`/etc/ssh/sshd_config`, which is the one a search finds first, changes the text and nothing else. **The relay bridge and the jump bridge are the same class and not the same lifetime.** A ticket is single-use with a 30-second expiry; a bastion's forward lives as long as the session. Sharing the listener @@ -164,8 +206,11 @@ is right, sharing a lifetime policy is not. ## Tests -- A two-hop connect against the Testcontainers `sshd`, which `Client.Ssh.Tests` already stands up — one - container as bastion, one as target, with the target refusing connections from anywhere else. +- ✅ A connect through a real SOCKS5 forward to a real `sshd`, in `LoopbackProxyTests`. **One container, + used as both ends** — the forward is opened on a connection to the fixture's server and the connection + under test goes back to the same server through it. A second container would look more like the topology + and establish nothing extra: what is under test is that the proxy is honoured, that the target is what + gets pinned, and that a failure on the way through is reported as itself. - A cycle in a chain is refused before any socket is opened. - Each hop's host key is asked about separately, and the question names the hop. - A chain crossing a vault is refused with a reason, as the group picker's is. @@ -199,3 +244,10 @@ The lesson is narrower than "read the ADRs": it is that *nothing reads this fiel field was a mistake, when it was evidence of an unfinished feature — and the same reasoning applied one paragraph further would have found the relay checkbox, which is the same shape and is actively lying to users. + +**And then step 1 was wrong too**, in the same direction: it named the bridge as the shared foundation on +the strength of ADR 0004's "one mechanism, two features", without checking which half of the mechanism +SSH.NET already provides. It provides the jump host's half entirely. The shared piece was one level down and +a tenth of the size, and the bridge belongs with the relay. Both corrections came from writing the code +rather than from reading more — which is an argument for the step-at-a-time ordering rather than against +planning, but only if the plan is edited when a step answers back. diff --git a/src/DodoSSH.Client.Ssh/SshConnection.cs b/src/DodoSSH.Client.Ssh/SshConnection.cs index 4a8bd1a..a81a9a3 100644 --- a/src/DodoSSH.Client.Ssh/SshConnection.cs +++ b/src/DodoSSH.Client.Ssh/SshConnection.cs @@ -16,18 +16,55 @@ public sealed record SshPasswordCredential(string Password) : SshCredential; /// Passphrase protecting the key, when it has one. public sealed record SshPrivateKeyCredential(byte[] PrivateKeyPem, string? Passphrase) : SshCredential; +/// +/// A SOCKS5 proxy on this machine's loopback interface, through which a host is reached. +/// +/// The port it is listening on. +/// +/// +/// ◆ A port and nothing else, so a proxy anywhere but loopback cannot be expressed. Both things +/// that will produce one of these listen on 127.0.0.1 — SSH.NET's own dynamic forward over a +/// bastion, and the bridge that will front the server relay — and a SOCKS proxy bound to any other +/// interface is an open proxy into whatever network the machine is on, for as long as the shell is up. +/// Leaving the host out of this type is what makes that unrepresentable rather than merely unlikely; it is +/// the same reason AuthenticationChoice carries a kind beside its id. +/// +/// +/// SOCKS5 rather than a plain pipe, and that is what keeps host key pinning honest. The target's +/// real name and port stay in and +/// and travel to the proxy in the CONNECT request, so the connection is *about* the target throughout — +/// nothing downstream has to be told that the address dialled is not the address being spoken to. A dumb +/// pipe would have meant handing SSH.NET 127.0.0.1 and remembering, everywhere else, that it was a +/// stand-in. See the gate in SshNetConnectionFactory, which pins what this request names. +/// +/// +/// Nothing in this assembly opens one. The proxy is somebody else's — a forward on a bastion connection, +/// or the relay bridge — and its lifetime belongs to whoever opened it, which must outlast the connection +/// made through it. See docs/reaching-a-host-you-cannot-dial.md. +/// +/// +public sealed record SshLoopbackProxy(int Port); + /// Everything needed to reach one host. /// Hostname or address. /// Port. /// Remote account. /// How to authenticate. /// How long to wait for the transport and handshake. +/// +/// A loopback SOCKS5 proxy to reach through, or null to dial it directly. +/// +/// Last and optional, so that every existing caller — which is every connection this product makes today — +/// keeps meaning exactly what it did. A host that can be dialled is still dialled. +/// +/// public sealed record SshConnectionRequest( string Host, int Port, string Username, SshCredential Credential, - TimeSpan? ConnectTimeout = null); + TimeSpan? ConnectTimeout = null, + SshLoopbackProxy? Proxy = null); /// An interactive shell over a pseudo-terminal. public interface ISshShellSession : IAsyncDisposable diff --git a/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs b/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs index d818786..54778ed 100644 --- a/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs +++ b/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs @@ -29,6 +29,16 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts) /// private const uint SftpBufferSize = 64 * 1024; + /// Where a is, and the only address one is ever dialled at. + /// + /// The literal rather than IPAddress.Loopback.ToString(), and rather than "localhost": SSH.NET + /// takes the proxy host as a string and resolves it, so a name would put a DNS lookup — and whatever + /// the machine's hosts file says localhost means — inside the connect path of every proxied + /// connection. It is also the half of the loopback promise this assembly can keep on its own; the other + /// half is that the proxy bound there, which is the caller's to get right. + /// + private const string LoopbackAddress = "127.0.0.1"; + /// public async Task ConnectAsync( SshConnectionRequest request, @@ -189,9 +199,23 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts) } /// + /// /// The known-host lookup inside the synchronous event is the one place this design cannot avoid /// blocking. It is a local store read rather than a UI round trip, and the alternative — making /// the store synchronous — would rule out the encrypted vault-backed implementation entirely. + /// + /// + /// ◆ A proxied connection differs here and nowhere else. The host, the port, the account and the + /// credential are the target's either way, and so is everything the gate above reads — which is what + /// makes a host reached through a bastion or a relay get pinned under its own name rather than under + /// 127.0.0.1. The proxy is a route, not a destination, and this is the one method that needs to + /// know the difference. See . + /// + /// + /// Timeout is assigned after the branch rather than in two initialisers, because it covers the + /// whole of getting there — the proxy handshake included — and having it stated once is what stops the + /// two paths quietly drifting to different waits. + /// /// private static ConnectionInfo BuildConnectionInfo(SshConnectionRequest request) { @@ -208,10 +232,27 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts) $"Credential type {request.Credential.GetType().Name} is not supported."), }; - return new ConnectionInfo(request.Host, request.Port, request.Username, method) - { - Timeout = request.ConnectTimeout ?? DefaultConnectTimeout, - }; + var info = request.Proxy is { } proxy + ? new ConnectionInfo( + request.Host, + request.Port, + request.Username, + ProxyTypes.Socks5, + LoopbackAddress, + proxy.Port, + + // No proxy credentials, and empty rather than null: SSH.NET offers username/password + // authentication to a SOCKS5 server only when it has been given one, and both proxies this + // client will ever use are on its own loopback interface, where a password would be a + // secret shared between two halves of the same process. + string.Empty, + string.Empty, + method) + : new ConnectionInfo(request.Host, request.Port, request.Username, method); + + info.Timeout = request.ConnectTimeout ?? DefaultConnectTimeout; + + return info; } private static PrivateKeyFile CreatePrivateKeyFile(SshPrivateKeyCredential credential) diff --git a/tests/DodoSSH.Client.Ssh.Tests/LoopbackProxyTests.cs b/tests/DodoSSH.Client.Ssh.Tests/LoopbackProxyTests.cs new file mode 100644 index 0000000..317c47e --- /dev/null +++ b/tests/DodoSSH.Client.Ssh.Tests/LoopbackProxyTests.cs @@ -0,0 +1,225 @@ +using System.Net; +using System.Net.Sockets; +using Renci.SshNet; + +namespace DodoSSH.Client.Ssh.Tests; + +/// +/// Reaching a host through a SOCKS5 proxy on loopback, which is how this client will reach one it cannot +/// dial: through a bastion, or through the server relay. +/// +/// +/// +/// One container, used as both ends. The fixture's sshd is the bastion and the target — a +/// dynamic forward is opened on a connection to it, and the connection under test goes back to the same +/// server through that forward. Two containers would look more like the real topology and would test +/// nothing extra: what is being established is that the request's proxy is honoured, that the target is +/// what gets pinned, and that a failure on the way through is reported as itself. None of the three is +/// about the far end being a different machine. +/// +/// +/// The forward is SSH.NET's own ForwardedPortDynamic, which is what the jump-host path will use in +/// earnest — so this is not a stub standing in for the eventual proxy, it is the eventual proxy. The relay +/// will put a bridge of this repository's own on the same loopback interface and speak the same protocol +/// to it. See docs/reaching-a-host-you-cannot-dial.md. +/// +/// +[Collection(SshCollection.Name)] +public sealed class LoopbackProxyTests(SshServerFixture fixture) +{ + private static CancellationToken Token => TestContext.Current.CancellationToken; + + /// + /// The whole of what this change buys, and the property that makes it safe. + /// + /// + /// + /// A connection through the proxy has to arrive, and it has to arrive as the target. The pin is + /// keyed on the host and port the request names, so if the proxy's address leaked into that identity + /// every machine reached through a bastion would be pinned as 127.0.0.1 on whatever ephemeral + /// port that day's forward happened to get — which is not an identity at all, and would mean a trusted + /// first contact for anything reached the same way afterwards. + /// + /// + /// ◆ The target is named as the bastion can reach it, not as this machine can. The forward runs + /// inside the container, so the address in the CONNECT request is resolved there — + /// localhost:2222 — and the published port this test host would use means nothing in that + /// namespace. That is not a quirk of the fixture: it is what ProxyJump means, and it is why an + /// ssh_config writes the target's *internal* address beside its jump host. Getting it wrong is a + /// SOCKS "general failure" from the bastion, which is what the first draft of this test collected. + /// + /// + /// Started from an empty store, so the assertion is not merely that the right string was recorded: the + /// same server is unknown under this identity until it is trusted under it, and being trusted under its + /// direct name would not do. Both halves of that are the point. + /// + /// + [Fact] + public async Task AHostReachedThroughAProxy_ConnectsAndIsPinnedUnderItsOwnName() + { + using var bastion = OpenBastion(); + using var forward = StartDynamicForward(bastion); + + var request = ThroughTheBastion(forward); + + var knownHosts = new InMemoryKnownHostStore(); + var factory = new SshNetConnectionFactory(knownHosts); + + var unknown = await Should.ThrowAsync(async () => + await factory.ConnectAsync(request, Token)); + + unknown.Presentation.Host.ShouldBe(InternalHost, "the target's name, not the proxy's"); + unknown.Presentation.Port.ShouldBe(SshServerFixture.InternalPort); + ((int)forward.BoundPort).ShouldNotBe( + SshServerFixture.InternalPort, "or the two identities would be indistinguishable"); + + await knownHosts.TrustAsync(unknown.Presentation, Token); + + await using var connection = await factory.ConnectAsync(request, Token); + + connection.IsConnected.ShouldBeTrue(); + connection.HostKey.Host.ShouldBe(InternalHost); + + // Authenticated is not the same as usable, and a proxied transport is exactly where a channel might + // not open: everything from here is SSH.NET's own framing over a socket it did not dial itself. + await using var shell = await connection.OpenShellAsync(TerminalSize.Default, Token); + + shell.IsOpen.ShouldBeTrue(); + } + + /// + /// The forward binds an ephemeral port and reports it, which is the one thing about + /// ForwardedPortDynamic this code depends on and the XML documentation does not state. Held here + /// so that an SSH.NET that stopped filling it in fails by name instead of leaving the test above + /// dialling port zero and reporting a connection error. + /// + [Fact] + public void ADynamicForward_ReportsThePortItWasGiven() + { + using var bastion = OpenBastion(); + using var forward = StartDynamicForward(bastion); + + forward.BoundHost.ShouldBe("127.0.0.1", "a SOCKS proxy on any other interface is an open proxy"); + forward.BoundPort.ShouldBeGreaterThan(0u, "an ephemeral bind has to report what it got"); + } + + /// + /// A proxy that is not there is a connection failure, and must not be dressed up as a host key problem. + /// + /// + /// The same misreport KeyAuthenticationTests guards for authentication, one layer lower and + /// easier to get wrong: the gate translates a refusal into on + /// the strength of having seen no host key, and a connection that never reached a server has seen none + /// either. Showing a fingerprint prompt for an unreachable bastion would offer to fix the wrong thing — + /// and there would be no fingerprint to show. + /// + [Fact] + public async Task AProxyThatIsNotListening_FailsAsAConnectionErrorRatherThanAnUnknownHostKey() + { + // An empty store, so the wrong answer is available: had the connection reached a server, this is + // exactly the setup that produces SshHostKeyUnknownException. It never gets that far. + var request = Request(Credential(), new SshLoopbackProxy(DeadPort())); + + var failure = await Should.ThrowAsync(async () => + await new SshNetConnectionFactory(new InMemoryKnownHostStore()).ConnectAsync(request, Token)); + + failure.ShouldNotBeOfType(); + failure.ShouldNotBeOfType(); + } + + /// + /// The connection every other test in this assembly makes, asserted once to be unchanged: the proxy is + /// an optional last parameter, so a request that names none has to build the connection it always did. + /// + [Fact] + public async Task AHostWithNoProxy_IsStillDialledDirectly() + { + var knownHosts = await TrustedStoreAsync(); + + await using var connection = await new SshNetConnectionFactory(knownHosts) + .ConnectAsync(Request(Credential(), proxy: null), Token); + + connection.IsConnected.ShouldBeTrue(); + } + + /// A port nothing is listening on, found by binding one and letting it go. + /// + /// Racy in principle and not in practice: nothing else in this process binds ephemeral ports, and the + /// consequence of losing the race is a connection that succeeds where the test wanted a refusal, which + /// fails the assertion rather than passing quietly. + /// + private static int DeadPort() + { + using var probe = new TcpListener(IPAddress.Loopback, 0); + + probe.Start(); + + var port = ((IPEndPoint)probe.LocalEndpoint).Port; + + probe.Stop(); + + return port; + } + + private SshClient OpenBastion() + { + var client = new SshClient( + fixture.Host, + fixture.Port, + SshServerFixture.Username, + SshServerFixture.Password); + + client.Connect(); + + return client; + } + + /// + /// Bound to 127.0.0.1 explicitly. The single-argument constructor's default is undocumented, and + /// the failure it would produce if that default is 0.0.0.0 is not a test failure — it is a SOCKS + /// proxy into the developer's network, open for as long as the connection lives, that nothing would + /// report. The test above asserts the bound host for the same reason. + /// + private static ForwardedPortDynamic StartDynamicForward(SshClient bastion) + { + var forward = new ForwardedPortDynamic("127.0.0.1", 0); + + bastion.AddForwardedPort(forward); + forward.Start(); + + return forward; + } + + /// What the container calls itself, which is the only name the forward inside it can resolve. + private const string InternalHost = "localhost"; + + private static SshPasswordCredential Credential() => new(SshServerFixture.Password); + + private SshConnectionRequest Request(SshCredential credential, SshLoopbackProxy? proxy) => + new(fixture.Host, fixture.Port, SshServerFixture.Username, credential, ConnectTimeout: null, proxy); + + /// The same server, addressed as the machine running the forward can reach it. + private static SshConnectionRequest ThroughTheBastion(ForwardedPortDynamic forward) => + new( + InternalHost, + SshServerFixture.InternalPort, + SshServerFixture.Username, + Credential(), + ConnectTimeout: null, + new SshLoopbackProxy((int)forward.BoundPort)); + + /// A store that already trusts the container's host key, so first contact is not the subject. + /// Learned by being refused, which is the only way this client learns a host key. + private async Task TrustedStoreAsync() + { + var knownHosts = new InMemoryKnownHostStore(); + + var unknown = await Should.ThrowAsync(async () => + await new SshNetConnectionFactory(knownHosts) + .ConnectAsync(Request(Credential(), proxy: null), Token)); + + await knownHosts.TrustAsync(unknown.Presentation, Token); + + return knownHosts; + } +} diff --git a/tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs b/tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs index 44ce514..765cf21 100644 --- a/tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs +++ b/tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs @@ -27,6 +27,17 @@ public sealed class SshServerFixture : IAsyncLifetime /// Password for password authentication. public const string Password = "correct-horse-battery-staple"; + /// + /// The port sshd listens on inside the container, rather than the one it is published on. + /// + /// + /// What anything reaching this server from within the container's own network namespace has to use — + /// which includes a forward opened on a connection to it. is the mapped port on the + /// host and is not listening in there. See LoopbackProxyTests, where the difference is the whole + /// shape of the test. + /// + public const int InternalPort = SshPort; + private const int SshPort = 2222; private readonly SemaphoreSlim sftpGate = new(1, 1); @@ -69,6 +80,53 @@ public sealed class SshServerFixture : IAsyncLifetime .Build(); await container.StartAsync(); + await AllowTcpForwardingAsync(); + } + + /// + /// Lets this server open the direct-tcpip channels a forward is made of. + /// + /// + /// + /// ◆ The image ships AllowTcpForwarding no, and nothing says so at the point it bites. A + /// dynamic forward starts perfectly happily — it is a local listener, and opening it asks the server + /// nothing — and then every connection through it is refused when the channel is opened. SSH.NET + /// reports that as SOCKS5: General failure from the proxy, which names neither the server nor + /// the setting, and is what the first run of LoopbackProxyTests collected. + /// + /// + /// Patched after start rather than baked in, because the image's entrypoint writes its configuration + /// itself on every boot — a mounted file would be overwritten before sshd read it. sshd re-reads on + /// SIGHUP and applies the result to connections made after that, and the readiness wait has + /// already run, so nothing here races the boot. + /// + /// + /// ◆ /config/sshd/sshd_config, and there are two. The image also carries + /// /etc/ssh/sshd_config, which looks like the file to patch, reads identically, and is not the + /// one the running server was started with — patching it changes the text and nothing else, which is a + /// fix that appears to work and leaves the failure exactly where it was. Measured with find + /// rather than assumed, after the first version of this method did precisely that. + /// + /// + /// It is on for the whole assembly rather than for the one test that needs it. Forwarding is off in + /// this image as hardening, not as a behaviour worth reproducing: nothing else here opens a channel of + /// any kind, so allowing it changes what exactly one suite can do and what none of the others see. + /// + /// + private async Task AllowTcpForwardingAsync() + { + var result = await container!.ExecAsync([ + "sh", + "-c", + "sed -i 's/^AllowTcpForwarding no/AllowTcpForwarding yes/' /config/sshd/sshd_config" + + " && pkill -HUP sshd", + ]); + + if (result.ExitCode != 0) + { + throw new InvalidOperationException( + $"Could not enable TCP forwarding on the test server: {result.Stderr}"); + } } ///