Author SHA1 Message Date
jaap-jan 9755b5f6ae Merge pull request 'Stop the SSH suite's server refusing connections at random' (#4) from claude/ssh-fixture-hup-race into main
ci / build and test (push) Successful in 2m24s
ci / android head (push) Successful in 3m17s
ci / desktop nightly (push) Successful in 41s
ci / api image (push) Successful in 23s
Reviewed-on: #4
2026-08-10 09:56:28 +00:00
jaap-jan afc6a042f1 Merge pull request 'Let the host editor make the credential it is about to bind' (#5) from claude/host-credentials-saved-sets-1786d4 into main
ci / build and test (push) Canceled after 0s
ci / android head (push) Canceled after 0s
ci / desktop nightly (push) Canceled after 0s
ci / api image (push) Canceled after 0s
Reviewed-on: #5
2026-08-10 09:56:22 +00:00
jaap-jan e41eca01a8 Stop the SSH suite's server refusing connections at random
ci / build and test (pull_request) Successful in 2m21s
ci / desktop nightly (pull_request) Skipped
ci / android head (pull_request) Successful in 3m20s
ci / api image (pull_request) Successful in 4s
The suite fails intermittently with SshConnectionException "The connection
was closed by the remote host", within tens of milliseconds, on whichever
test happens to connect first. It has been seen in CI and reproduces
locally. This raises sshd's MaxStartups in the fixture, which is the most
likely cause and is worth doing regardless.

sshd's compiled-in default is 10:30:100: past ten unauthenticated
connections in flight it refuses new ones at random, thirty percent of the
time, rising to always at a hundred. The image ships the line commented
out, so that default was what ran. xUnit runs test classes in parallel and
most of the classes here open a connection, so ten in flight is reachable
during the opening seconds — and a refusal presents to the client exactly
as observed, because a dropped connection and a server that never answered
are indistinguishable from that end.

◆ IT IS A MITIGATION AND NOT A DEMONSTRATED CURE, AND THE COMMENT SAYS SO.

The flake rate could not be measured. On the Windows development machine
the identical unmodified suite ran 85/85 clean and, an hour later, failed
13 runs out of 15; a Linux container gave 30/30 clean and then failed on
the first run of the next batch. Docker throughput on that host swings far
enough to swamp the effect, so every before/after comparison taken there
was noise — including two that were briefly believed.

It is committed on the narrower argument that it is right either way. 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.

The other candidate was the reload window — pkill returns when SIGHUP is
delivered, not when sshd has finished closing its listeners and re-execing,
so a connection immediately afterwards can be refused the same way. A wait
that required three consecutive banner reads before returning 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. Both candidates, and how to
tell them apart with sshd's own log, are recorded in the fixture and in
docs/platform-flags.md.
2026-08-10 11:46:14 +02:00
jaap-jan e96d01aab9 Let the host editor make the credential it is about to bind
ci / build and test (pull_request) Successful in 2m12s
ci / desktop nightly (pull_request) Skipped
ci / android head (pull_request) Successful in 3m18s
ci / api image (pull_request) Successful in 4s
The authentication picker has listed saved credentials since they existed, but
making one meant leaving a half-typed host for the keychain screen and coming
back to find it gone. On the phone it was worse than a detour: that head has no
credential editor at all, so it could bind a host to a credential and never
produce one. + NEW CREDENTIAL opens a card under the picker — name, optional
username, password, notes — and ADD writes it and binds the host in one step.

A button beside the picker rather than an entry inside it. Every row of that
list is a binding a host can have, and "make a new one" is an action: as an
entry it would sit in the box afterwards describing a state no host can be in,
and cancelling the form would leave the picker showing it.

It carries its own five fields rather than reusing the keychain editor's, and
that is the load-bearing part. IsEditingCredential is what AVaultEditorIsInTheWay
asks about, so sharing it would have made the whole Vault screen refuse to open
an editor while this card sat open on the Hosts screen, with a status line
naming a form the user cannot see on a screen they are not looking at — the
exact failure that guard was split in two to end. A test pins it.

It writes to the keychain immediately, unlike every other field in this editor,
because a credential is a shared item with an id and a host can only name an id
that exists. The consequence is honest rather than hidden and the hint says so:
a credential added this way outlives a cancelled host edit. What was still being
typed does not — every path that closes the host editor clears the form, and one
of those fields is a password.

The binding is written before the reload rather than after it. RefreshOpenEditors
rebuilds this picker and then restores it from the editor's own selection, so
setting it first is what survives the pass, and by the time it is read
ReloadCredentialsAsync has put the matching entry in the list to land on.

A name already taken is duplicated, not reused, and that is a deliberate parting
from the new-tag box six lines further down which offers the existing tag
instead. Two tags called "staging" are one intention spelled twice; two
credentials called "root" are two different passwords, and quietly binding the
host to whichever was there already would authenticate it as an account nobody
chose. A duplicate label in the picker is the smaller problem.

Into editingHostVaultId, so the credential lands wherever the host is being
sealed and everybody who can read the host can read what it authenticates with.
Stricter than the tag path — which files into the active vault and is recorded
as a gap in docs/design-import-gaps.md — and it can be, because this picker
lists credentials from every readable vault rather than one.

Five flow tests cover the bind-through-reload path, the cancel semantics on both
the saved credential and the abandoned one, the cross-screen guard, the
duplicate name and the empty-password refusal. The layout test is separate and
necessary: the card is collapsed until somebody presses the button, so a harness
driven by the default state draws none of it, and TheHostDrawerFitsWithTheHostEditorOpen
would have gone on passing over a card that blew the column. 1,854 tests, none
failing.
2026-08-10 11:23:10 +02:00
7 changed files with 497 additions and 1 deletions
+13
View File
@@ -388,6 +388,19 @@ agent of our own plus ProxyJump covers the real use cases.
**The SSH suite pulls `linuxserver/openssh-server` from Docker Hub**, which is rate-limited for **The SSH suite pulls `linuxserver/openssh-server` from Docker Hub**, which is rate-limited for
unauthenticated pulls. If CI starts failing on image pulls rather than on tests, that is why. unauthenticated pulls. If CI starts failing on image pulls rather than on tests, that is why.
**That suite has an intermittent `The connection was closed by the remote host`**, on whichever test
connects first, within tens of milliseconds. Seen in CI and reproducible locally. *Mitigated, not
solved:* `SshServerFixture` now raises sshd's `MaxStartups` from its compiled-in `10:30:100`, which
refuses connections at random past ten unauthenticated ones in flight — reachable because xUnit runs
test classes in parallel and most of them connect. The fixture comment carries the full argument and
is explicit that the cure is unproven.
**And the reason it is unproven is a measurement trap worth not falling into twice.** Docker
throughput on the Windows development machine swings enough to swamp the effect: the identical
unmodified suite ran 85/85 clean and, an hour later, failed 13 runs out of 15. Any before/after flake
comparison taken there is noise. Measure this class of thing in CI, or make the server say why —
raise sshd's `LogLevel`, disable Ryuk so the container outlives the run, and read `docker logs`.
**MSIX packaging is ruled out, not merely deprioritised.** A packaged app runs WebView2 in an **MSIX packaging is ruled out, not merely deprioritised.** A packaged app runs WebView2 in an
AppContainer where loopback connections are blocked without a `CheckNetIsolation` exemption. The AppContainer where loopback connections are blocked without a `CheckNetIsolation` exemption. The
terminal data plane *is* a loopback WebSocket, so MSIX would break the product outright. Velopack terminal data plane *is* a loopback WebSocket, so MSIX would break the product outright. Velopack
@@ -1029,6 +1029,48 @@
</ComboBox.ItemTemplate> </ComboBox.ItemTemplate>
</ComboBox> </ComboBox>
<!--
Making a credential without leaving the host, as on the desktop and on the same reasoning: the
moment one is wanted is while deciding how a host authenticates, and this head has no keychain
editor for credentials at all — so without this a phone could bind a host to a credential but
never make one. Writes to the keychain the instant ADD is pressed, exactly as the new-tag box
below does and for the same reason: a host can only name an id that exists.
-->
<Button Classes="secondary" Content="+ NEW CREDENTIAL" HorizontalAlignment="Left"
MinHeight="40" Padding="14,0"
IsVisible="{Binding !IsAddingEditorCredential}"
Command="{Binding BeginEditorCredentialCommand}" />
<Border CornerRadius="12" Background="{StaticResource Field}"
BorderBrush="{StaticResource Border}" BorderThickness="1" Padding="12"
IsVisible="{Binding IsAddingEditorCredential}">
<StackPanel Spacing="8">
<TextBlock Classes="label" Text="NEW CREDENTIAL" />
<TextBox Classes="field" Text="{Binding EditorNewCredentialLabel}"
PlaceholderText="name" />
<!--
Optional, and what makes a credential its own item: one account on twenty machines is
rotated in one place. Left blank, this host's own username is used.
-->
<TextBox Classes="field" Text="{Binding EditorNewCredentialUsername}"
PlaceholderText="username (blank: this host's own)" />
<TextBox Classes="field secret" Text="{Binding EditorNewCredentialPassword}"
PlaceholderText="password" />
<TextBox Classes="field" Text="{Binding EditorNewCredentialNotes}"
PlaceholderText="notes" />
<TextBlock Classes="detail" TextWrapping="Wrap"
Text="Added to the keychain as soon as you press ADD, so it stays even if you leave this host without saving." />
<Grid ColumnDefinitions="*,8,*">
<Button Grid.Column="0" Classes="primary" Content="ADD" MinHeight="44"
HorizontalAlignment="Stretch" HorizontalContentAlignment="Center"
Command="{Binding AddEditorCredentialCommand}" />
<Button Grid.Column="2" Classes="secondary" Content="CANCEL" MinHeight="44"
HorizontalAlignment="Stretch" HorizontalContentAlignment="Center"
Command="{Binding CancelEditorCredentialCommand}" />
</Grid>
</StackPanel>
</Border>
<!-- <!--
◆ WHICH VAULT THIS HOST WILL LIVE IN. Drawn only while adding and only where there is more than ◆ WHICH VAULT THIS HOST WILL LIVE IN. Drawn only while adding and only where there is more than
one vault that can be written to, exactly as on the desktop — an existing host's vault is not a one vault that can be written to, exactly as on the desktop — an existing host's vault is not a
@@ -584,6 +584,56 @@
</ComboBox.ItemTemplate> </ComboBox.ItemTemplate>
</ComboBox> </ComboBox>
<!--
Making a credential without leaving the host. The moment one is wanted is this one: somebody
is deciding how a host authenticates and finds the password is not in the keychain yet, and
sending them to the other screen to add it would lose the half-typed host they are standing
in. Same argument as the new-tag box further down, same immediate write, same honest
consequence — the credential stays if this editor is cancelled, because a host can only name
an id that exists.
A button beside the picker rather than an entry inside it. Every row of that list is a
binding the host can have; "make a new one" is an action, and as an entry it would sit in the
box afterwards describing a state no host can be in.
-->
<Button Classes="ghost" Content="+ NEW CREDENTIAL" HorizontalAlignment="Left"
FontSize="10.5" Height="28" Padding="10,0"
IsVisible="{Binding !IsAddingEditorCredential}"
Command="{Binding BeginEditorCredentialCommand}"
ToolTip.Tip="Adds a credential to the keychain and binds this host to it" />
<Border CornerRadius="12" Background="{StaticResource Field}"
BorderBrush="{StaticResource Border}" BorderThickness="1" Padding="12"
IsVisible="{Binding IsAddingEditorCredential}">
<StackPanel Spacing="6">
<TextBlock Classes="label" Text="NEW CREDENTIAL" FontSize="10" />
<TextBox Text="{Binding EditorNewCredentialLabel}" PlaceholderText="name" Height="36" />
<!--
Optional, and what makes a credential worth being its own item: one account on twenty
machines is rotated in one place. Left blank, this host's own username is used.
-->
<TextBox Text="{Binding EditorNewCredentialUsername}" Height="36"
PlaceholderText="username (blank: use this host's own)" />
<!-- Masked, on the reasoning the keychain's own password box carries. -->
<TextBox Text="{Binding EditorNewCredentialPassword}" PlaceholderText="password"
PasswordChar="•" Height="36">
<TextBox.KeyBindings>
<KeyBinding Gesture="Enter" Command="{Binding AddEditorCredentialCommand}" />
</TextBox.KeyBindings>
</TextBox>
<TextBox Text="{Binding EditorNewCredentialNotes}" PlaceholderText="notes"
AcceptsReturn="True" Height="44" TextWrapping="Wrap" />
<TextBlock Classes="hint" FontSize="10.5" TextWrapping="Wrap"
Text="Added to the keychain as soon as you press ADD, so it stays even if you cancel this host. Renaming and deleting are on the keychain screen." />
<StackPanel Orientation="Horizontal" Spacing="6">
<Button Classes="accent" Content="ADD"
Command="{Binding AddEditorCredentialCommand}" />
<Button Classes="ghost" Content="CANCEL"
Command="{Binding CancelEditorCredentialCommand}" />
</StackPanel>
</StackPanel>
</Border>
<!-- <!--
◆ THE RELAY CARD, restyled to the mock's nested-card shape — radius 12, a checkbox with the ◆ THE RELAY CARD, restyled to the mock's nested-card shape — radius 12, a checkbox with the
title beside it rather than under it — but NOT to the mock's copy. The sentence stays title beside it rather than under it — but NOT to the mock's copy. The sentence stays
@@ -3086,6 +3086,37 @@ internal sealed partial class VaultViewModel(
[ObservableProperty] [ObservableProperty]
private AuthenticationChoice? editorSelectedAuthentication; private AuthenticationChoice? editorSelectedAuthentication;
// ---- Making a credential from inside the host editor ----
// A fifth set of editor fields, and deliberately not the keychain screen's four. Sharing them would put
// IsEditingCredential — which AVaultEditorIsInTheWay asks about — true while the user is on the Hosts
// screen, and the whole Vault screen would refuse to open an editor with a sentence naming a form on
// another screen. That is the exact failure AHostEditorIsInTheWay was split out to end; see its remarks.
/// <summary>Whether the host editor is showing its own new-credential form.</summary>
[ObservableProperty]
private bool isAddingEditorCredential;
/// <summary>The name in the host editor's new-credential form.</summary>
[ObservableProperty]
private string editorNewCredentialLabel = string.Empty;
/// <inheritdoc cref="CredentialEditorUsername" path="/remarks" />
[ObservableProperty]
private string editorNewCredentialUsername = string.Empty;
/// <remarks>
/// Holds a password for as long as the form is open, on the same terms the keychain's box does — see
/// <see cref="CredentialEditorPassword"/>. Cleared by every path that closes this form, including the
/// ones that close the host editor around it, so a password typed here cannot outlive the form and
/// reappear behind the next host somebody edits.
/// </remarks>
[ObservableProperty]
private string editorNewCredentialPassword = string.Empty;
/// <summary>Free text, as the keychain's own editor takes.</summary>
[ObservableProperty]
private string editorNewCredentialNotes = string.Empty;
/// <summary>What the group picker offers: "no group", then every group of the chosen vault.</summary> /// <summary>What the group picker offers: "no group", then every group of the chosen vault.</summary>
/// <inheritdoc cref="EditorAuthenticationChoices" path="/remarks" /> /// <inheritdoc cref="EditorAuthenticationChoices" path="/remarks" />
internal ObservableCollection<GroupChoice> EditorGroupChoices { get; } = []; internal ObservableCollection<GroupChoice> EditorGroupChoices { get; } = [];
@@ -3357,6 +3388,121 @@ internal sealed partial class VaultViewModel(
OnPropertyChanged(nameof(HasTagChoices)); OnPropertyChanged(nameof(HasTagChoices));
} }
/// <summary>
/// Opens the host editor's own new-credential form.
/// </summary>
/// <remarks>
/// A button beside the picker rather than an entry inside it. Every row of that list is a binding the
/// host can have — see <see cref="AuthenticationChoice"/> — and "make a new one" is an action, not a
/// binding: as an entry it would sit in the box afterwards describing a state no host can be in, and
/// cancelling the form would leave the picker showing it.
/// </remarks>
[RelayCommand]
private void BeginEditorCredential()
{
ClearEditorCredentialForm();
IsAddingEditorCredential = true;
Status = "Adding a credential for this host.";
}
/// <summary>Abandons the form, clearing the password out of it.</summary>
[RelayCommand]
private void CancelEditorCredential()
{
ClearEditorCredentialForm();
Status = string.Empty;
}
/// <summary>Closes the form and drops what was typed into it, the password included.</summary>
private void ClearEditorCredentialForm()
{
IsAddingEditorCredential = false;
EditorNewCredentialLabel = string.Empty;
EditorNewCredentialUsername = string.Empty;
EditorNewCredentialPassword = string.Empty;
EditorNewCredentialNotes = string.Empty;
}
/// <summary>
/// Creates a credential from the host editor's form and binds the host being edited to it.
/// </summary>
/// <remarks>
/// <para>
/// The same reasoning <see cref="AddEditorTagAsync"/> gives, and for the same moment: somebody is
/// choosing how a host authenticates and finds the password they want is not in the keychain yet.
/// Sending them to the other screen to make one would lose the half-typed host they were standing in.
/// </para>
/// <para>
/// <b>It writes to the keychain immediately, unlike every other field in this editor.</b> A credential
/// is a shared item with an id and a host can only name an id that exists, so there is nothing to defer.
/// Cancelling the host edit therefore leaves the credential behind — honest rather than hidden, and the
/// bargain a tag already makes here.
/// </para>
/// <para>
/// <b>A name that already exists is duplicated rather than reused, which is where this deliberately
/// parts from the tag path.</b> Two tags called "staging" are the same intention spelled twice; two
/// credentials called "root" are two different passwords, and quietly binding the host to the one that
/// happened to be there already would authenticate it as an account the user never chose. A duplicate
/// label in the picker is a smaller problem than a silent wrong password.
/// </para>
/// <para>
/// Into <see cref="editingHostVaultId"/>, not the standing target: the credential belongs wherever the
/// host is being sealed, so everybody who can read the host can read what it authenticates with. That is
/// stricter than the tag path — which files into the active vault and is recorded as a gap — and it can
/// be, because the picker here lists credentials from every readable vault rather than one.
/// </para>
/// </remarks>
[RelayCommand]
private async Task AddEditorCredentialAsync(CancellationToken cancellationToken)
{
var credential = new CredentialSecret
{
Label = EditorNewCredentialLabel.Trim(),
// Not trimmed. A password of spaces is a password — CredentialSecret.TryValidate says so — and
// trimming one here would lock somebody out of a host over a tidiness opinion.
Password = EditorNewCredentialPassword,
Username = string.IsNullOrWhiteSpace(EditorNewCredentialUsername)
? null
: EditorNewCredentialUsername.Trim(),
// Untrimmed and unnormalised past blank-is-absent, as the keychain's editor writes it: free text
// is the user's to lay out, and its leading indent is theirs rather than this form's to correct.
Notes = string.IsNullOrWhiteSpace(EditorNewCredentialNotes) ? null : EditorNewCredentialNotes,
};
if (!credential.TryValidate(out var reason))
{
Status = reason;
return;
}
await RunAsync(
"Saving…",
async () =>
{
var entityId = await session.Credentials
.CreateAsync(editingHostVaultId, credential, cancellationToken)
.ConfigureAwait(true);
// Before the reload, not after it. RefreshOpenEditors rebuilds this picker and then restores
// it from whatever this property says, so writing the binding here is what survives the pass
// — and by the time it is read, ReloadCredentialsAsync has put the matching entry in the
// list for it to land on.
EditorSelectedAuthentication =
AuthenticationChoice.ForCredential(entityId, credential.Label);
ClearEditorCredentialForm();
await ReloadAsync(cancellationToken).ConfigureAwait(true);
Status = $"Added '{credential.Label}' and bound this host to it. "
+ "Save the host to keep the binding.";
}).ConfigureAwait(true);
await AutoSyncAsync(cancellationToken).ConfigureAwait(true);
}
/// <summary> /// <summary>
/// The paths pinned on the host being edited, in the order QUICK ACCESS draws them. /// The paths pinned on the host being edited, in the order QUICK ACCESS draws them.
/// </summary> /// </summary>
@@ -7092,6 +7238,9 @@ internal sealed partial class VaultViewModel(
EditorPinnedPaths.Clear(); EditorPinnedPaths.Clear();
EditorNewPin = string.Empty; EditorNewPin = string.Empty;
// Closed rather than carried over, and it holds a password — see EditorNewCredentialPassword.
ClearEditorCredentialForm();
// Before the group picker, because a group belongs to one vault and the picker is that vault's. // Before the group picker, because a group belongs to one vault and the picker is that vault's.
BuildEditorVaultChoices(editingHostVaultId); BuildEditorVaultChoices(editingHostVaultId);
@@ -7177,6 +7326,9 @@ internal sealed partial class VaultViewModel(
EditorNewTag = string.Empty; EditorNewTag = string.Empty;
BuildTagChoices(); BuildTagChoices();
// As in NewHost, and for the password it can be holding.
ClearEditorCredentialForm();
LoadEditorPinnedPaths(row.Host.PinnedPaths); LoadEditorPinnedPaths(row.Host.PinnedPaths);
BuildEditorVaultChoices(editingHostVaultId); BuildEditorVaultChoices(editingHostVaultId);
@@ -8037,6 +8189,10 @@ internal sealed partial class VaultViewModel(
{ {
IsEditing = false; IsEditing = false;
editingEntityId = null; editingEntityId = null;
// The form goes with the editor it lives in, password and all. A credential already added through it
// stays in the keychain — see AddEditorCredentialAsync — but what was still being typed does not.
ClearEditorCredentialForm();
Status = string.Empty; Status = string.Empty;
} }
@@ -8070,6 +8226,10 @@ internal sealed partial class VaultViewModel(
} }
IsEditing = false; IsEditing = false;
// As CancelEdit does, for the same password.
ClearEditorCredentialForm();
await ReloadAsync(cancellationToken).ConfigureAwait(true); await ReloadAsync(cancellationToken).ConfigureAwait(true);
SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == editingEntityId); SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == editingEntityId);
@@ -189,6 +189,25 @@ public sealed class ScreenLayoutTests : IAsyncLifetime
await MeasureDrawerAsync(faults => faults.ShouldBeEmpty()); await MeasureDrawerAsync(faults => faults.ShouldBeEmpty());
} }
/// <remarks>
/// The editor with its new-credential card showing, which the test above never draws: the card is
/// collapsed until somebody presses + NEW CREDENTIAL, so nothing else in this suite measures the three
/// boxes, the paragraph of hint text and the two buttons it adds inside the section that already holds
/// the authentication picker. A card that only appears on a click is exactly the shape that escapes a
/// harness driven by the default state.
/// </remarks>
[Fact]
public async Task TheHostDrawerFitsWithTheNewCredentialFormOpen()
{
vault.SelectedHost = vault.Hosts[0];
vault.EditSelectedHostCommand.Execute(null);
vault.BeginEditorCredentialCommand.Execute(null);
vault.IsAddingEditorCredential.ShouldBeTrue("there is nothing to measure otherwise");
await MeasureDrawerAsync(faults => faults.ShouldBeEmpty());
}
/// <remarks> /// <remarks>
/// The other editor, and it is in this control for the first time: the desktop's group editor used to be /// The other editor, and it is in this control for the first time: the desktop's group editor used to be
/// a bar across the foot of the hosts screen, where it competed with the grid for the same column. Its /// a bar across the foot of the hosts screen, where it competed with the grid for the same column. Its
@@ -2980,6 +2980,161 @@ public sealed class ShellFlowTests : IAsyncLifetime
vault.Hosts[0].Host.CredentialId.ShouldBeNull(); vault.Hosts[0].Host.CredentialId.ShouldBeNull();
} }
/// <remarks>
/// The moment a credential is wanted is the moment somebody is choosing how a host authenticates and
/// finds it is not in the keychain yet, so the host editor makes one. Selecting it has to survive the
/// reload the write triggers, which is the part that needs a test: the refill rebuilds the picker from
/// the vault and restores it from the editor's own selection, so the binding is written before the
/// reload rather than after it.
/// </remarks>
[Fact]
public async Task ACredentialMadeInTheHostEditor_BindsTheHostToIt()
{
var vault = await ReadyToConnectAsync();
vault.SelectedHost = vault.Hosts[0];
vault.EditSelectedHostCommand.Execute(null);
vault.BeginEditorCredentialCommand.Execute(null);
vault.IsAddingEditorCredential.ShouldBeTrue();
vault.EditorNewCredentialLabel = "pg-primary";
vault.EditorNewCredentialUsername = "postgres";
vault.EditorNewCredentialPassword = "s3cret";
vault.EditorNewCredentialNotes = "rotated quarterly";
await vault.AddEditorCredentialCommand.ExecuteAsync(null);
var credential = vault.Credentials.ShouldHaveSingleItem();
credential.Credential.Username.ShouldBe("postgres");
credential.Credential.Notes.ShouldBe("rotated quarterly");
vault.IsAddingEditorCredential.ShouldBeFalse("the form closes once the credential is in the keychain");
vault.EditorNewCredentialPassword.ShouldBeEmpty("the form must not go on holding the password");
vault.EditorSelectedAuthentication.ShouldNotBeNull().EntityId.ShouldBe(
credential.EntityId,
"the picker has to land on the credential that was just made, through the reload");
await vault.SaveHostCommand.ExecuteAsync(null);
vault.Hosts.ShouldHaveSingleItem().Host.CredentialId.ShouldBe(credential.EntityId);
vault.Hosts[0].Host.SshKeyId.ShouldBeNull();
}
/// <remarks>
/// The honest consequence of writing immediately, and the same one the new-tag box already carries: a
/// credential is a shared item with an id, the host can only name an id that exists, so the credential
/// was never part of the host to begin with. What was still being typed is a different matter — that
/// includes a password, and it goes with the editor it was typed into.
/// </remarks>
[Fact]
public async Task CancellingTheHostEditor_KeepsTheCredentialItMade_AndDropsWhatWasStillBeingTyped()
{
var vault = await ReadyToConnectAsync();
vault.SelectedHost = vault.Hosts[0];
vault.EditSelectedHostCommand.Execute(null);
vault.BeginEditorCredentialCommand.Execute(null);
vault.EditorNewCredentialLabel = "pg-primary";
vault.EditorNewCredentialPassword = "s3cret";
await vault.AddEditorCredentialCommand.ExecuteAsync(null);
// A second one, opened and left half-typed.
vault.BeginEditorCredentialCommand.Execute(null);
vault.EditorNewCredentialLabel = "half";
vault.EditorNewCredentialPassword = "typed-but-never-added";
vault.EditorNewCredentialNotes = "half a thought";
vault.CancelEditCommand.Execute(null);
vault.Credentials.ShouldHaveSingleItem().Label.ShouldBe("pg-primary");
vault.IsAddingEditorCredential.ShouldBeFalse();
vault.EditorNewCredentialLabel.ShouldBeEmpty();
vault.EditorNewCredentialNotes.ShouldBeEmpty();
vault.EditorNewCredentialPassword.ShouldBeEmpty(
"a password typed into an abandoned form must not survive behind the next host");
vault.Hosts.ShouldHaveSingleItem().Host.CredentialId.ShouldBeNull(
"the binding itself was never saved");
}
/// <remarks>
/// Why this form has fields of its own rather than reusing the keychain screen's four.
/// <c>IsEditingCredential</c> is what <c>AVaultEditorIsInTheWay</c> asks about, so sharing it would make
/// the whole Vault screen refuse to open an editor, with a sentence naming a form the user cannot see
/// on a screen they are not looking at. That is the exact failure the guard was split in two to end.
/// </remarks>
[Fact]
public async Task TheHostEditorsCredentialForm_DoesNotBlockTheKeychainsOwnEditors()
{
var vault = await ReadyToConnectAsync();
vault.SelectedHost = vault.Hosts[0];
vault.EditSelectedHostCommand.Execute(null);
vault.BeginEditorCredentialCommand.Execute(null);
vault.NewCredentialCommand.Execute(null);
vault.IsEditingCredential.ShouldBeTrue(
"the keychain's editor lives on another screen and opens regardless");
}
/// <remarks>
/// Where this deliberately parts from the new-tag box beside it, which offers an existing tag rather
/// than repeating it. Two tags called "staging" are one intention spelled twice; two credentials called
/// "root" are two different passwords, and quietly binding the host to whichever was there already
/// would authenticate it as an account nobody chose.
/// </remarks>
[Fact]
public async Task ACredentialMadeInTheHostEditor_UnderANameAlreadyTaken_IsASecondCredential()
{
var vault = await ReadyToConnectAsync();
await AddCredentialAsync(vault, "root", password: "first");
var first = vault.Credentials.ShouldHaveSingleItem().EntityId;
vault.SelectedHost = vault.Hosts[0];
vault.EditSelectedHostCommand.Execute(null);
vault.BeginEditorCredentialCommand.Execute(null);
vault.EditorNewCredentialLabel = "root";
vault.EditorNewCredentialPassword = "second";
await vault.AddEditorCredentialCommand.ExecuteAsync(null);
vault.Credentials.Count.ShouldBe(2);
vault.EditorSelectedAuthentication.ShouldNotBeNull().EntityId.ShouldNotBe(
first,
"binding to the credential that happened to share the name would be the wrong password");
}
/// <remarks>
/// The same refusal <c>CredentialSecret.TryValidate</c> gives the keychain's editor, reaching the user
/// here rather than producing an item that looks usable and fails at the handshake.
/// </remarks>
[Fact]
public async Task ACredentialMadeInTheHostEditor_WithNoPassword_IsRefused()
{
var vault = await ReadyToConnectAsync();
vault.SelectedHost = vault.Hosts[0];
vault.EditSelectedHostCommand.Execute(null);
vault.BeginEditorCredentialCommand.Execute(null);
vault.EditorNewCredentialLabel = "pg-primary";
await vault.AddEditorCredentialCommand.ExecuteAsync(null);
vault.Credentials.ShouldBeEmpty();
vault.IsAddingEditorCredential.ShouldBeTrue("the form stays open on what it refused");
vault.Status.ShouldContain("password");
}
/// <remarks> /// <remarks>
/// Tags reach the same editor by a different route — the keychain screen rather than the box under the /// Tags reach the same editor by a different route — the keychain screen rather than the box under the
/// chips — and a chip that only appeared on the next open would send the user round the same detour. /// chips — and a chip that only appeared on the next open would send the user round the same detour.
@@ -112,6 +112,61 @@ public sealed class SshServerFixture : IAsyncLifetime
/// this image as hardening, not as a behaviour worth reproducing: nothing else here opens a channel of /// 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. /// any kind, so allowing it changes what exactly one suite can do and what none of the others see.
/// </para> /// </para>
/// <para>
/// ◆ <b><c>MaxStartups</c> is raised here too, against a flake this suite has and that this change is
/// a mitigation for rather than a proven cure.</b> The distinction is stated because the evidence
/// 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>
/// </remarks> /// </remarks>
private async Task AllowTcpForwardingAsync() private async Task AllowTcpForwardingAsync()
{ {
@@ -119,14 +174,16 @@ public sealed class SshServerFixture : IAsyncLifetime
"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"
+ " && pkill -HUP sshd", + " && pkill -HUP sshd",
]); ]);
if (result.ExitCode != 0) if (result.ExitCode != 0)
{ {
throw new InvalidOperationException( throw new InvalidOperationException(
$"Could not enable TCP forwarding on the test server: {result.Stderr}"); $"Could not reconfigure the test server: {result.Stderr}");
} }
} }
/// <summary> /// <summary>