Public Access
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
b80bf23341 | ||
|
|
8c58e5a558 | ||
|
|
53ff15ba86 | ||
|
|
766fe6aebe |
@@ -11247,10 +11247,7 @@ internal sealed partial class VaultViewModel(
|
|||||||
}
|
}
|
||||||
catch (TimeoutException)
|
catch (TimeoutException)
|
||||||
{
|
{
|
||||||
Abandon(
|
Abandon(attempt, RendererNeverStarted);
|
||||||
attempt,
|
|
||||||
"The terminal did not start, so nothing was connected. The Microsoft Edge WebView2 "
|
|
||||||
+ "runtime is probably missing or blocked; install it and try again.");
|
|
||||||
}
|
}
|
||||||
catch (SshHostKeyUnknownException exception)
|
catch (SshHostKeyUnknownException exception)
|
||||||
{
|
{
|
||||||
@@ -11275,6 +11272,29 @@ internal sealed partial class VaultViewModel(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>What a renderer that never attached is reported as.</summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// The wait is translated rather than reported for the reason <see cref="OpenSessionAsync"/> gives —
|
||||||
|
/// <see cref="TimeoutException"/> says only "The operation has timed out" — and the whole value of the
|
||||||
|
/// translation is naming where to look. Which is why it cannot be one sentence: the desktop's answer is
|
||||||
|
/// a runtime this application does not install, and the phone has no such runtime and no such answer.
|
||||||
|
/// Telling somebody on a handset to install Microsoft Edge WebView2 is worse than saying nothing, at the
|
||||||
|
/// one moment they are trying to work out what went wrong.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// A runtime check rather than a constructor parameter, for the reason
|
||||||
|
/// <c>MainWindowViewModel.GestureWait</c> records at length: which renderer is behind the terminal is a
|
||||||
|
/// fact about the platform this assembly is running on, not about one installation of it.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
private static string RendererNeverStarted =>
|
||||||
|
OperatingSystem.IsAndroid()
|
||||||
|
? "The terminal did not start, so nothing was connected. Android's WebView is probably "
|
||||||
|
+ "disabled or updating; check it in Settings and try again."
|
||||||
|
: "The terminal did not start, so nothing was connected. The Microsoft Edge WebView2 "
|
||||||
|
+ "runtime is probably missing or blocked; install it and try again.";
|
||||||
|
|
||||||
/// <summary>Says, in one place, that an attempt ended without a session and why.</summary>
|
/// <summary>Says, in one place, that an attempt ended without a session and why.</summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// The reason goes to two places on purpose. The status line is where somebody watching this screen is
|
/// The reason goes to two places on purpose. The status line is where somebody watching this screen is
|
||||||
|
|||||||
@@ -84,14 +84,57 @@ const RELEASE_FOCUS_MESSAGE = 'dodossh.release-focus';
|
|||||||
const root = document.getElementById('root');
|
const root = document.getElementById('root');
|
||||||
const statusBanner = document.getElementById('status');
|
const statusBanner = document.getElementById('status');
|
||||||
|
|
||||||
/** @type {Map<number, {term: object, fit: object, pane: HTMLElement}>} */
|
/** @type {Map<number, {term: object, fit: object, pane: HTMLElement, notice: string}>} */
|
||||||
const sessions = new Map();
|
const sessions = new Map();
|
||||||
|
|
||||||
/** @type {WebSocket | null} */
|
/** @type {WebSocket | null} */
|
||||||
let socket = null;
|
let socket = null;
|
||||||
|
|
||||||
function setStatus(text) {
|
/** Whose pane is showing, or null before there is one — see activate(). */
|
||||||
statusBanner.textContent = text ?? '';
|
let activeSessionId = null;
|
||||||
|
|
||||||
|
/*
|
||||||
|
── THE BANNER BELONGS TO ONE PANE AT A TIME ─────────────────────────────────────────────────────────
|
||||||
|
There is one #status element for the whole page, because there is one page for every terminal: the
|
||||||
|
panes are stacked in the same box and all but the active one are hidden. What goes in it comes from
|
||||||
|
two sources that are not the same size, and the difference is the whole of this.
|
||||||
|
|
||||||
|
The socket's troubles are the page's. There is a single socket behind every pane, so "the view is
|
||||||
|
reconnecting" is true of whatever is on screen and true of the panes behind it.
|
||||||
|
|
||||||
|
A session's last words are not. "The remote closed the session." is a fact about one terminal and says
|
||||||
|
nothing whatever about the others — so it is held on the session and drawn only while that session's
|
||||||
|
pane is the one showing. Written straight into the shared element, which is what this used to do, it
|
||||||
|
outlived the tab it described: switching to a live terminal left the dead one's epitaph sitting under
|
||||||
|
it, and opening or closing any other tab wiped the message whether or not it belonged to that tab.
|
||||||
|
|
||||||
|
The socket's half wins when both have something to say: a page whose socket is down is not showing
|
||||||
|
live output on any pane, which makes what became of one session the less urgent of the two.
|
||||||
|
*/
|
||||||
|
let transportStatus = statusBanner.textContent ?? '';
|
||||||
|
|
||||||
|
function renderStatus() {
|
||||||
|
const notice = activeSessionId === null ? '' : sessions.get(activeSessionId)?.notice ?? '';
|
||||||
|
|
||||||
|
statusBanner.textContent = transportStatus || notice;
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Says something about the socket, which every pane shares. */
|
||||||
|
function setTransportStatus(text) {
|
||||||
|
transportStatus = text ?? '';
|
||||||
|
renderStatus();
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Records what became of one session, to be drawn only while that session's pane is showing. */
|
||||||
|
function setSessionNotice(sessionId, text) {
|
||||||
|
const session = sessions.get(sessionId);
|
||||||
|
|
||||||
|
if (!session) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
session.notice = text ?? '';
|
||||||
|
renderStatus();
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Builds a frame: opcode, big-endian session id, then payload. */
|
/** Builds a frame: opcode, big-endian session id, then payload. */
|
||||||
@@ -255,8 +298,31 @@ function createSession(sessionId) {
|
|||||||
// WebGL where it is available. Falling back rather than failing matters because a software
|
// WebGL where it is available. Falling back rather than failing matters because a software
|
||||||
// renderer is slow but usable, whereas a blank pane is not — and remote desktops and VMs
|
// renderer is slow but usable, whereas a blank pane is not — and remote desktops and VMs
|
||||||
// routinely have no usable GPU context.
|
// routinely have no usable GPU context.
|
||||||
|
//
|
||||||
|
// ◆ THE CONTEXT-LOSS HANDLER IS THE HALF THAT WAS MISSING, AND ON A PHONE IT IS THE WHOLE THING.
|
||||||
|
//
|
||||||
|
// The addon does not recover from a lost GPU context by itself, and it does not fail loudly either:
|
||||||
|
// it stays loaded over a dead context and draws nothing at all. What that looks like from outside is
|
||||||
|
// a terminal that is connected, still accepting keystrokes, still acknowledging output — and blank.
|
||||||
|
// xterm's own guidance is to dispose the addon and let the DOM renderer take over, which is what this
|
||||||
|
// does; the addon is not reloaded afterwards, because a pane that lost the context once is on a
|
||||||
|
// surface that will do it again and thrashing between renderers is worse than being slow.
|
||||||
|
//
|
||||||
|
// Losing it is ordinary on Android and nearly unheard of on Windows, which is why this went unnoticed
|
||||||
|
// for so long. Collapsing the renderer sets the native view to GONE — see
|
||||||
|
// AndroidNativeControlHostImpl.HideWithSize — and a WebView with no surface has no GL context. The
|
||||||
|
// shell collapses it every time a tab starts connecting, every time the connect sheet opens and every
|
||||||
|
// time the app is backgrounded, so on a phone the first loss arrives within seconds of the first
|
||||||
|
// session. WebView2 hides a child HWND instead and keeps rendering throughout; see
|
||||||
|
// docs/platform-flags.md.
|
||||||
try {
|
try {
|
||||||
term.loadAddon(new WebglAddon.WebglAddon());
|
const webgl = new WebglAddon.WebglAddon();
|
||||||
|
|
||||||
|
// Subscribed before loadAddon, because loadAddon is what activates the addon and a context that is
|
||||||
|
// already gone can be reported from inside that call.
|
||||||
|
webgl.onContextLoss(() => webgl.dispose());
|
||||||
|
|
||||||
|
term.loadAddon(webgl);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.warn('WebGL renderer unavailable; falling back to canvas.', error);
|
console.warn('WebGL renderer unavailable; falling back to canvas.', error);
|
||||||
}
|
}
|
||||||
@@ -269,7 +335,7 @@ function createSession(sessionId) {
|
|||||||
|
|
||||||
term.onResize(() => sendResize(sessionId, term, pane));
|
term.onResize(() => sendResize(sessionId, term, pane));
|
||||||
|
|
||||||
const session = { term, fit, pane };
|
const session = { term, fit, pane, notice: '' };
|
||||||
sessions.set(sessionId, session);
|
sessions.set(sessionId, session);
|
||||||
|
|
||||||
activate(sessionId);
|
activate(sessionId);
|
||||||
@@ -283,6 +349,11 @@ function activate(sessionId) {
|
|||||||
session.pane.dataset.active = String(id === sessionId);
|
session.pane.dataset.active = String(id === sessionId);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The banner follows the pane. Whatever this session has to say for itself replaces whatever the
|
||||||
|
// session that was showing had to say for its own, which is the point of holding it per session.
|
||||||
|
activeSessionId = sessionId;
|
||||||
|
renderStatus();
|
||||||
|
|
||||||
const active = sessions.get(sessionId);
|
const active = sessions.get(sessionId);
|
||||||
if (active) {
|
if (active) {
|
||||||
active.term.focus();
|
active.term.focus();
|
||||||
@@ -296,10 +367,17 @@ function activate(sessionId) {
|
|||||||
// caller, because more than one path reaches here: a minimised window, and a splitter dragged to the edge
|
// caller, because more than one path reaches here: a minimised window, and a splitter dragged to the edge
|
||||||
// once splits land.
|
// once splits land.
|
||||||
//
|
//
|
||||||
// It is *not* what protects the vault's lock screen, which an earlier version of this comment claimed.
|
// It is *not* what protects the vault's lock screen on the desktop, which an earlier version of this
|
||||||
// Collapsing the host's WebView hides a native child window without resizing it, so this page's viewport
|
// comment claimed. Collapsing WebView2 hides a native child window without resizing it, so this page's
|
||||||
// does not change, no observer fires and this function is never called — measured with a live shell, and
|
// viewport does not change, no observer fires and this function is never called — measured with a live
|
||||||
// confirmed by removing the guard and finding the lock cycle equally clean. See docs/platform-flags.md.
|
// shell, and confirmed by removing the guard and finding the lock cycle equally clean. See
|
||||||
|
// docs/platform-flags.md.
|
||||||
|
//
|
||||||
|
// On the phone it *is* load-bearing, and that is the one place the two heads differ here. Android hides a
|
||||||
|
// native child by setting it GONE, and a GONE view is skipped by its parent's layout — so collapsing the
|
||||||
|
// renderer really does take this page's viewport to nothing, the observer really does fire, and without
|
||||||
|
// the guard every lock, every connect sheet and every trip to the background would reflow the remote pty
|
||||||
|
// to 2x1 and mangle the scrollback it wrapped.
|
||||||
const MINIMUM_FITTABLE_PIXELS = 40;
|
const MINIMUM_FITTABLE_PIXELS = 40;
|
||||||
|
|
||||||
function resize(session, sessionId) {
|
function resize(session, sessionId) {
|
||||||
@@ -343,7 +421,10 @@ function handleFrame(buffer) {
|
|||||||
session.term.write(REPLAY_BANNER);
|
session.term.write(REPLAY_BANNER);
|
||||||
}
|
}
|
||||||
|
|
||||||
setStatus('');
|
// This session's own line, and only this one's: a session that is open has nothing to say about
|
||||||
|
// how it ended. The page's own "Connecting…" is cleared by the socket opening, which happens
|
||||||
|
// before any frame can arrive.
|
||||||
|
setSessionNotice(sessionId, '');
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -396,7 +477,14 @@ function handleFrame(buffer) {
|
|||||||
session.pane.remove();
|
session.pane.remove();
|
||||||
sessions.delete(sessionId);
|
sessions.delete(sessionId);
|
||||||
|
|
||||||
setStatus('');
|
// The notice went with the session record it was held on, but the page can still be pointing at
|
||||||
|
// the pane that is now gone. Cleared rather than left dangling, so the banner stops describing a
|
||||||
|
// closed tab while the host decides which pane to show next.
|
||||||
|
if (activeSessionId === sessionId) {
|
||||||
|
activeSessionId = null;
|
||||||
|
}
|
||||||
|
|
||||||
|
renderStatus();
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -473,14 +561,19 @@ function handleFrame(buffer) {
|
|||||||
const session = sessions.get(sessionId);
|
const session = sessions.get(sessionId);
|
||||||
const reason = new TextDecoder().decode(payload);
|
const reason = new TextDecoder().decode(payload);
|
||||||
|
|
||||||
if (session) {
|
if (!session) {
|
||||||
// The pane and its scrollback stay. The user was probably reading the last thing the
|
// No pane, so there is nothing this page can honestly hang the reason on. It used to go into
|
||||||
// remote said, and that is usually why the session ended.
|
// the banner anyway, which printed one session's ending underneath whichever pane happened to
|
||||||
session.term.write(`\r\n\x1b[38;5;244m── ${reason} ──\x1b[0m\r\n`);
|
// be showing at the time.
|
||||||
session.term.options.cursorBlink = false;
|
break;
|
||||||
}
|
}
|
||||||
|
|
||||||
setStatus(reason);
|
// The pane and its scrollback stay. The user was probably reading the last thing the
|
||||||
|
// remote said, and that is usually why the session ended.
|
||||||
|
session.term.write(`\r\n\x1b[38;5;244m── ${reason} ──\x1b[0m\r\n`);
|
||||||
|
session.term.options.cursorBlink = false;
|
||||||
|
|
||||||
|
setSessionNotice(sessionId, reason);
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -505,7 +598,7 @@ function scheduleReconnect() {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
setStatus('Reconnecting the terminal view…');
|
setTransportStatus('Reconnecting the terminal view…');
|
||||||
|
|
||||||
reconnectTimer = setTimeout(() => {
|
reconnectTimer = setTimeout(() => {
|
||||||
reconnectTimer = null;
|
reconnectTimer = null;
|
||||||
@@ -525,7 +618,7 @@ function connect() {
|
|||||||
socket.binaryType = 'arraybuffer';
|
socket.binaryType = 'arraybuffer';
|
||||||
|
|
||||||
socket.addEventListener('open', () => {
|
socket.addEventListener('open', () => {
|
||||||
setStatus('');
|
setTransportStatus('');
|
||||||
|
|
||||||
// Back to the quick attempt for whatever the next failure turns out to be. Kept slow between
|
// Back to the quick attempt for whatever the next failure turns out to be. Kept slow between
|
||||||
// attempts within one outage, reset once the outage is actually over.
|
// attempts within one outage, reset once the outage is actually over.
|
||||||
|
|||||||
@@ -1,4 +1,6 @@
|
|||||||
|
using System.Net.Sockets;
|
||||||
using System.Security.Cryptography;
|
using System.Security.Cryptography;
|
||||||
|
using System.Text;
|
||||||
using DotNet.Testcontainers.Builders;
|
using DotNet.Testcontainers.Builders;
|
||||||
using DotNet.Testcontainers.Containers;
|
using DotNet.Testcontainers.Containers;
|
||||||
using Xunit;
|
using Xunit;
|
||||||
@@ -40,6 +42,21 @@ public sealed class SshServerFixture : IAsyncLifetime
|
|||||||
|
|
||||||
private const int SshPort = 2222;
|
private const int SshPort = 2222;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// How many connections in a row the server has to answer before this fixture calls it ready.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// Twenty-five, and the number is measured rather than picked. Probing a fresh container 200 times with
|
||||||
|
/// penalties left at the image's default, the first <c>Not allowed at this time</c> came back at probe
|
||||||
|
/// 18 and 183 of the 200 were refused; with <c>PerSourcePenalties no</c> applied, none of 200 were. Ten
|
||||||
|
/// was tried first and is useless — it sits below the threshold, so the guard passed happily against a
|
||||||
|
/// server that was still penalising. See <see cref="WaitUntilServingAsync"/>.
|
||||||
|
/// </remarks>
|
||||||
|
private const int RequiredStreak = 25;
|
||||||
|
|
||||||
|
/// <summary>How long to keep trying before giving up on the server entirely.</summary>
|
||||||
|
private static readonly TimeSpan ReadyTimeout = TimeSpan.FromSeconds(60);
|
||||||
|
|
||||||
private readonly SemaphoreSlim sftpGate = new(1, 1);
|
private readonly SemaphoreSlim sftpGate = new(1, 1);
|
||||||
|
|
||||||
private IContainer? container;
|
private IContainer? container;
|
||||||
@@ -80,101 +97,94 @@ public sealed class SshServerFixture : IAsyncLifetime
|
|||||||
.Build();
|
.Build();
|
||||||
|
|
||||||
await container.StartAsync();
|
await container.StartAsync();
|
||||||
await AllowTcpForwardingAsync();
|
await ReconfigureAsync();
|
||||||
|
await WaitUntilServingAsync();
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Lets this server open the direct-tcpip channels a forward is made of.
|
/// Turns off the hardening this suite trips over, and makes the running server re-read its config.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// ◆ <b>The image ships <c>AllowTcpForwarding no</c>, and nothing says so at the point it bites.</b> A
|
/// ◆ <b><c>PerSourcePenalties no</c> is the fix for the flake this suite had for months, and the other
|
||||||
/// dynamic forward starts perfectly happily — it is a local listener, and opening it asks the server
|
/// two settings here are not.</b> OpenSSH 9.8 added per-source penalties and 10.x has them on by
|
||||||
/// nothing — and then every connection through it is refused when the channel is opened. SSH.NET
|
/// default; this image runs 10.3. A source address that keeps disconnecting without authenticating is
|
||||||
/// reports that as <c>SOCKS5: General failure</c> from the proxy, which names neither the server nor
|
/// penalised, and while the penalty holds every connection from it is answered with the clear-text line
|
||||||
/// the setting, and is what the first run of <c>LoopbackProxyTests</c> collected.
|
/// <c>Not allowed at this time</c> and then closed.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// Patched after start rather than baked in, because the image's entrypoint writes its configuration
|
/// <b>This suite generates exactly that traffic, by design.</b> This client's first contact with an
|
||||||
/// itself on every boot — a mounted file would be overwritten before sshd read it. sshd re-reads on
|
/// unknown host is a connection deliberately refused at the host key — which is a disconnect with no
|
||||||
/// <c>SIGHUP</c> and applies the result to connections made after that, and the readiness wait has
|
/// authentication attempt — and several tests do nothing else:
|
||||||
/// already run, so nothing here races the boot.
|
/// <c>RefusingTheHostKey_AbortsTheConnection</c>, <c>AnUntrustedHost_IsRefusedExactlyAsAShellWouldBe</c>,
|
||||||
|
/// and every helper that learns a host key by being turned away first. Enough of them close together and
|
||||||
|
/// sshd stops talking to the test host altogether, for a while, and then starts again.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// From the client that is <c>SshConnectionException: The connection was closed by the remote host</c>
|
||||||
|
/// within milliseconds — no banner, nothing to say which of the many reasons it was. It hits whichever
|
||||||
|
/// class is running when the penalty lands and spares the rest, which is why it read as random and why
|
||||||
|
/// the class it hit lost <em>every</em> connection it made rather than a random few. The one test in that
|
||||||
|
/// class that expects a refusal passed throughout, for the wrong reason.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// ◆ <b>Two earlier diagnoses were wrong, and are recorded here so they are not tried again.</b>
|
||||||
|
/// <c>MaxStartups</c> was blamed on the reasoning that xUnit runs test classes in parallel, so ten
|
||||||
|
/// unauthenticated connections would be in flight at once — but every class that touches this server
|
||||||
|
/// shares <see cref="SshCollection"/>, and xUnit's unit of parallelism is the collection, so they run one
|
||||||
|
/// after another and never have more than a connection or two open. The reload window was blamed next,
|
||||||
|
/// and a wait for the banner to answer was written and removed as unproven; it was unproven because the
|
||||||
|
/// banner does answer, right up until the penalty lands.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// The line is appended rather than replaced in place, unlike the two below it, because the image's
|
||||||
|
/// config does not mention the keyword at all — there is no line to replace, and sshd takes the first
|
||||||
|
/// value it finds for a keyword that appears more than once.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// ◆ <b><c>AllowTcpForwarding</c> is what a dynamic forward needs</b>, and the image ships it off as
|
||||||
|
/// hardening. Without it a forward opens perfectly happily — a local listener asks the server nothing —
|
||||||
|
/// and then every connection through it is refused when the channel is opened. SSH.NET reports that as
|
||||||
|
/// <c>SOCKS5: General failure</c>, which names neither the server nor the setting, and is what the first
|
||||||
|
/// run of <c>LoopbackProxyTests</c> collected. That suite is also the alarm if this method ever silently
|
||||||
|
/// stops working.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// <c>MaxStartups</c> is raised for the reason it should have been in the first place rather than as a
|
||||||
|
/// fix for anything: the compiled-in default refuses connections at random past ten unauthenticated ones
|
||||||
|
/// in flight, and a throttle is hardening a test server has no business reproducing. It is kept, not
|
||||||
|
/// because it was ever shown to matter here, but because removing it would be a second change riding
|
||||||
|
/// along with this one.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Both are replaced in place rather than appended, because sshd_config takes the <em>first</em> value
|
||||||
|
/// it finds for a keyword: an appended line would be dead the day the image ships an uncommented one of
|
||||||
|
/// its own.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// ◆ <b><c>/config/sshd/sshd_config</c>, and there are two.</b> The image also carries
|
/// ◆ <b><c>/config/sshd/sshd_config</c>, and there are two.</b> The image also carries
|
||||||
/// <c>/etc/ssh/sshd_config</c>, which looks like the file to patch, reads identically, and is not the
|
/// <c>/etc/ssh/sshd_config</c>, which looks like the file to patch, reads identically, and is not the one
|
||||||
/// one the running server was started with — patching it changes the text and nothing else, which is a
|
/// the running server was started with — patching it changes the text and nothing else, which is a fix
|
||||||
/// fix that appears to work and leaves the failure exactly where it was. Measured with <c>find</c>
|
/// that appears to work and leaves the failure exactly where it was.
|
||||||
/// rather than assumed, after the first version of this method did precisely that.
|
|
||||||
/// </para>
|
/// </para>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// It is on for the whole assembly rather than for the one test that needs it. Forwarding is off in
|
/// ◆ <b>Patched after boot and reloaded, rather than injected before it — which was tried and does not
|
||||||
/// this image as hardening, not as a behaviour worth reproducing: nothing else here opens a channel of
|
/// work.</b> This image family runs <c>/custom-cont-init.d</c> scripts, which look like the right hook
|
||||||
/// any kind, so allowing it changes what exactly one suite can do and what none of the others see.
|
/// and are not: the container's own log puts <c>sshd is listening on port 2222</c> <em>before</em>
|
||||||
/// </para>
|
/// <c>[custom-init] Files found, executing</c>, so a script there edits a file the running server has
|
||||||
/// <para>
|
/// already read. It leaves a config that greps correctly and a server behaving as though it had never
|
||||||
/// ◆ <b><c>MaxStartups</c> is raised here too, against a flake this suite has and that this change is
|
/// been touched — the same trap as the wrong file, one layer up. Measured from the log, after a version
|
||||||
/// a mitigation for rather than a proven cure.</b> The distinction is stated because the evidence
|
/// of this fixture did exactly that and failed twenty-eight tests.
|
||||||
/// stops short of the claim, and a later reader deserves to know which.
|
|
||||||
/// </para>
|
|
||||||
/// <para>
|
|
||||||
/// What is established: sshd's compiled-in default is <c>10:30:100</c> — past ten
|
|
||||||
/// <em>unauthenticated</em> connections in flight it refuses new ones at random, thirty percent of the
|
|
||||||
/// time, rising to always at a hundred — and the image ships the line commented out, so that default
|
|
||||||
/// was what ran. xUnit runs test classes in parallel and most classes here open a connection, so ten
|
|
||||||
/// in flight is reachable in the opening seconds. A refused connection presents to the client as
|
|
||||||
/// <c>SshConnectionException: The connection was closed by the remote host</c> within tens of
|
|
||||||
/// milliseconds, on whichever test connects at the wrong moment — which is exactly the observed
|
|
||||||
/// failure, seen in CI and reproduced locally.
|
|
||||||
/// </para>
|
|
||||||
/// <para>
|
|
||||||
/// What is <em>not</em> established is that this limit is the only cause, because the flake rate could
|
|
||||||
/// not be measured reliably. On the development machine the identical unmodified suite ran 85/85 clean
|
|
||||||
/// and, an hour later, failed 13 runs out of 15 — Docker throughput on that host swings far enough to
|
|
||||||
/// swamp the effect being measured. Any before/after comparison taken there is noise, and two were,
|
|
||||||
/// before that was noticed.
|
|
||||||
/// </para>
|
|
||||||
/// <para>
|
|
||||||
/// It is committed anyway, on the narrower argument that it is right regardless: a connection throttle
|
|
||||||
/// is hardening this suite has no interest in reproducing. It exists to test an SSH client, not to
|
|
||||||
/// survive a rate limit, and a test server that drops connections at random is a bad test server
|
|
||||||
/// whether or not it is the cause of this particular flake.
|
|
||||||
/// </para>
|
|
||||||
/// <para>
|
|
||||||
/// <b>Not fixed by serialising the suite</b>, which would have hidden it and cost the parallelism, and
|
|
||||||
/// not by retrying the connect, which would have made the client's own reconnect behaviour untestable
|
|
||||||
/// by burying it in the fixture. The limit is a property of a hardened server that this suite has no
|
|
||||||
/// interest in reproducing — it exists to test an SSH client, not to survive a throttle.
|
|
||||||
/// </para>
|
|
||||||
/// <para>
|
|
||||||
/// Replaced in place rather than appended, because sshd_config takes the <em>first</em> value it finds
|
|
||||||
/// for a keyword: an appended line would be dead the day the image ships an uncommented one of its own.
|
|
||||||
/// </para>
|
|
||||||
/// <para>
|
|
||||||
/// ◆ <b>The reload window is the other candidate, and it is deliberately not guarded against.</b>
|
|
||||||
/// <c>SIGHUP</c> makes sshd close its listeners and re-execute itself, and <c>pkill</c> returns when
|
|
||||||
/// the signal is delivered rather than when that has finished — so in principle a connection made
|
|
||||||
/// immediately afterwards is refused, producing this same exception. A wait that opened connections
|
|
||||||
/// until the server answered with its banner three times running was written, and then removed: it
|
|
||||||
/// could not be shown to change anything either, and a fixture carrying two unproven fixes for one
|
|
||||||
/// symptom is worse than one, because the next person has to disprove both.
|
|
||||||
/// </para>
|
|
||||||
/// <para>
|
|
||||||
/// If this flake returns, that is the next thing to try. Two things to know before trying it: the two
|
|
||||||
/// causes are indistinguishable from the client, so a fix can only be judged by a repeat run and never
|
|
||||||
/// by whether the next run passes — and the repeat run has to happen somewhere with stable Docker
|
|
||||||
/// throughput, which the development machine is not. Better still, make sshd say why: raise its
|
|
||||||
/// <c>LogLevel</c> here, disable Ryuk so the container outlives the run, and read
|
|
||||||
/// <c>docker logs</c>. A <c>MaxStartups</c> refusal names itself there; a reload does not.
|
|
||||||
/// </para>
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
private async Task AllowTcpForwardingAsync()
|
private async Task ReconfigureAsync()
|
||||||
{
|
{
|
||||||
var result = await container!.ExecAsync([
|
var result = await container!.ExecAsync([
|
||||||
"sh",
|
"sh",
|
||||||
"-c",
|
"-c",
|
||||||
"sed -i 's/^AllowTcpForwarding no/AllowTcpForwarding yes/' /config/sshd/sshd_config"
|
"sed -i 's/^AllowTcpForwarding no/AllowTcpForwarding yes/' /config/sshd/sshd_config"
|
||||||
+ " && sed -i 's/^#*MaxStartups .*/MaxStartups 200/' /config/sshd/sshd_config"
|
+ " && sed -i 's/^#*MaxStartups .*/MaxStartups 200/' /config/sshd/sshd_config"
|
||||||
|
+ " && printf '\\nPerSourcePenalties no\\n' >> /config/sshd/sshd_config"
|
||||||
+ " && pkill -HUP sshd",
|
+ " && pkill -HUP sshd",
|
||||||
]);
|
]);
|
||||||
|
|
||||||
@@ -183,7 +193,107 @@ public sealed class SshServerFixture : IAsyncLifetime
|
|||||||
throw new InvalidOperationException(
|
throw new InvalidOperationException(
|
||||||
$"Could not reconfigure the test server: {result.Stderr}");
|
$"Could not reconfigure the test server: {result.Stderr}");
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Blocks until the server answers <see cref="RequiredStreak"/> connections in a row with its banner.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// ◆ <b>This is a guard rather than a wait, and what it guards against is
|
||||||
|
/// <c>PerSourcePenalties</c> coming back.</b> Reconfiguring above turns it off; this proves it is off,
|
||||||
|
/// immediately and by name, instead of letting the suite discover it later as an unrelated-looking
|
||||||
|
/// failure in whichever class happened to be running.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// <b>Consecutive, and deliberately with no pause between them.</b> Each probe opens a connection, reads
|
||||||
|
/// the identification string and disconnects without authenticating — which is exactly the shape of
|
||||||
|
/// connection <c>PerSourcePenalties</c> punishes, and exactly what this suite does all day: a first
|
||||||
|
/// contact with an unknown host is a connection this client deliberately refuses at the host key.
|
||||||
|
/// <see cref="RequiredStreak"/> back to back is therefore not a soak test, it is the specific
|
||||||
|
/// provocation, sized above the measured threshold on purpose, and it costs well under a second when the
|
||||||
|
/// setting is off.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// It is also the one check that can tell a listening socket from a running server. The container's own
|
||||||
|
/// readiness — a log line and <c>netstat</c> showing <c>:2222</c> — passes on a container whose sshd has
|
||||||
|
/// gone: the socket is published by a host-side proxy that accepts before it has anything to forward to,
|
||||||
|
/// so a dead server presents as a connection accepted and closed rather than as one refused.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Probed from the host rather than with <c>docker exec</c>, deliberately: that is the path the tests
|
||||||
|
/// take, proxy included, and penalties are counted per source address — from inside the container the
|
||||||
|
/// source would be the loopback rather than the address every test connects from.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
private async Task WaitUntilServingAsync()
|
||||||
|
{
|
||||||
|
// TimeProvider.System rather than DateTimeOffset.UtcNow, which this repository bans so that time can
|
||||||
|
// be faked — and rather than a fake, because what is being waited on is a real container starting.
|
||||||
|
var deadline = TimeProvider.System.GetUtcNow() + ReadyTimeout;
|
||||||
|
var streak = 0;
|
||||||
|
var last = "no probe ran";
|
||||||
|
|
||||||
|
while (streak < RequiredStreak)
|
||||||
|
{
|
||||||
|
if (TimeProvider.System.GetUtcNow() >= deadline)
|
||||||
|
{
|
||||||
|
throw new InvalidOperationException(
|
||||||
|
$"The test server did not answer {RequiredStreak} connections in a row within "
|
||||||
|
+ $"{ReadyTimeout}. The last probe said: {last}. If it says \"Not allowed at this "
|
||||||
|
+ "time\", sshd is penalising this source address and PerSourcePenalties is no longer "
|
||||||
|
+ "being turned off — see ReconfigureAsync.");
|
||||||
|
}
|
||||||
|
|
||||||
|
var (answered, what) = await ProbeAsync();
|
||||||
|
last = what;
|
||||||
|
|
||||||
|
if (answered)
|
||||||
|
{
|
||||||
|
streak++;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Only pause when it is not working. Back-to-back probes are the point while they succeed;
|
||||||
|
// hammering a server that has not finished starting is just noise.
|
||||||
|
streak = 0;
|
||||||
|
await Task.Delay(TimeSpan.FromMilliseconds(200));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>Opens a socket and reads far enough to see OpenSSH's identification string.</summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// The description comes back with the answer because the interesting failures are not exceptions. A
|
||||||
|
/// penalised source is told <c>Not allowed at this time</c> in clear text before the socket closes, and
|
||||||
|
/// a suite that only knew "no banner" would have to go and find that out again — which is what happened
|
||||||
|
/// the first time, at some length.
|
||||||
|
/// </remarks>
|
||||||
|
private async Task<(bool Answered, string What)> ProbeAsync()
|
||||||
|
{
|
||||||
|
try
|
||||||
|
{
|
||||||
|
using var probe = new TcpClient();
|
||||||
|
using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(5));
|
||||||
|
|
||||||
|
await probe.ConnectAsync(Host, Port, timeout.Token);
|
||||||
|
|
||||||
|
var buffer = new byte[64];
|
||||||
|
var read = await probe.GetStream().ReadAtLeastAsync(
|
||||||
|
buffer, 4, throwOnEndOfStream: false, timeout.Token);
|
||||||
|
|
||||||
|
var answered = read >= 4 && "SSH-"u8.SequenceEqual(buffer.AsSpan(0, 4));
|
||||||
|
|
||||||
|
return (
|
||||||
|
answered,
|
||||||
|
answered
|
||||||
|
? "SSH-"
|
||||||
|
: $"{read} bytes: "
|
||||||
|
+ Encoding.ASCII.GetString(buffer, 0, Math.Max(read, 0)).ReplaceLineEndings(" "));
|
||||||
|
}
|
||||||
|
catch (Exception exception) when (exception is SocketException or OperationCanceledException or IOException)
|
||||||
|
{
|
||||||
|
return (false, $"{exception.GetType().Name}: {exception.Message}");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
@@ -225,12 +335,17 @@ public sealed class SshServerFixture : IAsyncLifetime
|
|||||||
/// </summary>
|
/// </summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// Shared rather than opened per test, and that is a limit of the server rather than an optimisation.
|
/// Shared rather than opened per test. This was once explained as a way of staying under the server's
|
||||||
/// sshd's <c>MaxStartups</c> drops connections at random once enough are part-way through a handshake,
|
/// <c>MaxStartups</c> throttle, on the belief that the suite ran its classes in parallel and made two
|
||||||
/// and this client's first contact with an unknown host is a connection deliberately <em>refused</em> at
|
/// handshakes per test — this client's first contact with an unknown host is a connection deliberately
|
||||||
/// the host key — so a suite that opened its own session per test made two handshakes per test and
|
/// <em>refused</em> at the host key, so every session costs two. The parallelism was not real: every
|
||||||
/// pushed the whole assembly over the threshold. What that looks like is unrelated tests failing with
|
/// class here shares one collection and xUnit runs collections, not classes, in parallel. See
|
||||||
/// "the connection was closed by the remote host", a different few each run.
|
/// <see cref="WaitUntilServingAsync"/>, which is where that mistake was found and what the failure it
|
||||||
|
/// was blamed for turned out to be.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// It stays shared regardless, on the plainer argument: one session is enough, and a handshake per test
|
||||||
|
/// would be seconds of the suite's runtime spent proving nothing this file has not already proved.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// Safe to share because an SFTP session holds no per-test state: every test here works in a directory
|
/// Safe to share because an SFTP session holds no per-test state: every test here works in a directory
|
||||||
|
|||||||
Reference in New Issue
Block a user