Public Access
Offer to bring the keys an ssh_config points at
An import that recorded a key path and left every host asking for a password was an import whose result did not connect. The answer to that was a manual paste per key, which is the sort of thing people do once and then stop importing. So there is a tick, and it starts off. With it off nothing changes: an IdentityFile becomes a note and the host asks for a password. With it on, IMPORT reads each host's first IdentityFile out of ~/.ssh, stores it in the vault encrypted like any other key, and binds the host to it. Three things about how it is drawn are load-bearing rather than tidy. It is a default nobody arrives at by accident. The sentence beside it names the directory rather than saying "your keys", because that is what somebody is agreeing to. And nothing is read during SCAN — tick it, read what it says, untick it, and no private key has been opened. This is the only place the application opens key material out of a directory the user did not point at file by file, and the whole of what makes that acceptable is that it took a deliberate press. One vault key per file, however many entries named it: an ssh_config pointing twelve hosts at one id_ed25519 is the ordinary shape, and twelve copies would be twelve things to rotate and eleven to forget. A file whose material is already in the keychain is bound to rather than stored again, which is what makes running the import twice harmless. What cannot be read off a disk is a passphrase, so a protected key arrives without one — and the report under the button names those files rather than leaving a host to fail at connect time with a message about a malformed key. Telling them apart means decoding for OpenSSH's own container, whose cipher name is the first field inside the base64 rather than anything in the armour, and that is the format ssh-keygen has written by default for years. The 88 base64 characters it decodes need 66 bytes, not 64: with the smaller span every protected key came back unprotected, which the tests now pin. A path that is not on this machine leaves its host imported and unbound, exactly as it would have been with the tick off, and is named in the same report. A config carried from another machine is the ordinary case, not an error.
This commit is contained in:
@@ -3480,6 +3480,230 @@ public sealed class ShellFlowTests : IAsyncLifetime
|
||||
server.LiveRowCount.ShouldBe(2);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// ◆ <b>The tick, and the half of it that matters is what happens with it off.</b> Reading a private key
|
||||
/// out of a home directory is the act this product exists to make deliberate, so the default has to be a
|
||||
/// default nobody arrives at by accident — and a scan that read keys in order to describe them would
|
||||
/// have already done the thing the tick gates. So this asserts twice over: nothing is bound, and the
|
||||
/// report that names every file it opened is empty.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task ImportingWithoutTheKeyTick_RecordsThePathAndReadsNothing()
|
||||
{
|
||||
await UnlockedAsync();
|
||||
var vault = shell.Vault!;
|
||||
|
||||
var sshDirectory = KeyedConfigDirectory();
|
||||
|
||||
var import = new ImportViewModel(vault, new SshConfigLocator(sshDirectory));
|
||||
|
||||
await import.ScanCommand.ExecuteAsync(null);
|
||||
|
||||
import.HasKeyFiles.ShouldBeTrue("the tick is drawn only where there is a key to read");
|
||||
import.ImportsKeys.ShouldBeFalse("and it starts off");
|
||||
|
||||
await import.ImportCommand.ExecuteAsync(null);
|
||||
|
||||
vault.Keys.ShouldBeEmpty("nothing reached into ~/.ssh");
|
||||
vault.Hosts.ShouldHaveSingleItem().Host.SshKeyId.ShouldBeNull();
|
||||
import.KeyReport.ShouldBeEmpty("and no file was opened to report on");
|
||||
|
||||
// The path is still recorded, which is what the import did before the tick existed.
|
||||
vault.Hosts[0].Host.Notes.ShouldContain("id_ed25519");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// The other side of it: with the tick on, the key is read, stored encrypted in the vault and bound to
|
||||
/// the host — which is the difference between an import whose result connects and one whose every host
|
||||
/// asks for a password.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task ImportingWithTheKeyTick_StoresTheKeyAndBindsTheHostToIt()
|
||||
{
|
||||
await UnlockedAsync();
|
||||
var vault = shell.Vault!;
|
||||
|
||||
var sshDirectory = KeyedConfigDirectory();
|
||||
|
||||
var import = new ImportViewModel(vault, new SshConfigLocator(sshDirectory));
|
||||
|
||||
await import.ScanCommand.ExecuteAsync(null);
|
||||
|
||||
import.ImportsKeys = true;
|
||||
|
||||
// The row says what it will do, which is the only place a person sees the tick's effect per host.
|
||||
import.Rows.ShouldHaveSingleItem().Authentication.ShouldContain("imported");
|
||||
|
||||
await import.ImportCommand.ExecuteAsync(null);
|
||||
|
||||
var key = vault.Keys.ShouldHaveSingleItem();
|
||||
|
||||
key.Key.PrivateKeyPem.ShouldContain("PRIVATE KEY");
|
||||
key.Key.PublicKey.ShouldBe("ssh-ed25519 AAAAC3Nz nobody@example");
|
||||
key.Key.Notes.ShouldContain("id_ed25519");
|
||||
|
||||
vault.Hosts.ShouldHaveSingleItem().Host.SshKeyId.ShouldBe(key.EntityId);
|
||||
vault.Hosts[0].Authentication.ShouldBe("key");
|
||||
|
||||
import.KeyReport.ShouldHaveSingleItem().ShouldContain("imported");
|
||||
import.Status.ShouldContain("1 private key");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// One vault key per file, however many entries named it — the shape a real <c>ssh_config</c> actually
|
||||
/// has. Twelve copies of one private key would be twelve things to rotate and eleven to forget, which is
|
||||
/// the argument <c>HostSecret.SshKeyId</c> already makes for referencing a key rather than embedding it.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The second half is the one that would go unnoticed: importing the same config twice must bind to the
|
||||
/// key that is already there rather than storing a second copy of it. Asserted by running the whole
|
||||
/// import again over a keychain that now holds one.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task TwoHostsNamingOneKeyFile_ShareOneStoredKeyAndARepeatAddsNone()
|
||||
{
|
||||
await UnlockedAsync();
|
||||
var vault = shell.Vault!;
|
||||
|
||||
var sshDirectory = KeyedConfigDirectory("""
|
||||
Host web-01
|
||||
HostName web-01.internal
|
||||
User deploy
|
||||
IdentityFile ~KEY~
|
||||
|
||||
Host web-02
|
||||
HostName web-02.internal
|
||||
User deploy
|
||||
IdentityFile ~KEY~
|
||||
""");
|
||||
|
||||
var import = new ImportViewModel(vault, new SshConfigLocator(sshDirectory));
|
||||
|
||||
await import.ScanCommand.ExecuteAsync(null);
|
||||
|
||||
import.ImportsKeys = true;
|
||||
|
||||
await import.ImportCommand.ExecuteAsync(null);
|
||||
|
||||
var key = vault.Keys.ShouldHaveSingleItem("one file is one key");
|
||||
|
||||
vault.Hosts.Count.ShouldBe(2);
|
||||
vault.Hosts.ShouldAllBe(row => row.Host.SshKeyId == key.EntityId);
|
||||
import.Status.ShouldContain("1 private key");
|
||||
|
||||
// Again, over a keychain that already holds it. The hosts duplicate — a second bookmark for one
|
||||
// machine is allowed and takes a click — and the key must not.
|
||||
foreach (var row in import.Rows)
|
||||
{
|
||||
row.IsSelected = true;
|
||||
}
|
||||
|
||||
await import.ImportCommand.ExecuteAsync(null);
|
||||
|
||||
vault.Keys.ShouldHaveSingleItem("the material was already here, so it was bound to rather than stored");
|
||||
import.Status.ShouldNotContain("private key", Case.Insensitive);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// A passphrase is the one thing about a key file that is not in the key file, so an encrypted one comes
|
||||
/// in without it and would fail at connect time with SSH.NET's own message. The import says so instead —
|
||||
/// naming the file, because a config with a dozen keys has a dozen candidates for which one it was.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task AnEncryptedKeyIsImportedAndSaidToBeEncrypted()
|
||||
{
|
||||
await UnlockedAsync();
|
||||
var vault = shell.Vault!;
|
||||
|
||||
var sshDirectory = KeyedConfigDirectory();
|
||||
|
||||
await File.WriteAllTextAsync(
|
||||
Path.Combine(sshDirectory, "id_ed25519"),
|
||||
"-----BEGIN RSA PRIVATE KEY-----\nProc-Type: 4,ENCRYPTED\nDEK-Info: AES-128-CBC,00\n\nQUJD\n"
|
||||
+ "-----END RSA PRIVATE KEY-----\n",
|
||||
Token);
|
||||
|
||||
var import = new ImportViewModel(vault, new SshConfigLocator(sshDirectory));
|
||||
|
||||
await import.ScanCommand.ExecuteAsync(null);
|
||||
|
||||
import.ImportsKeys = true;
|
||||
|
||||
await import.ImportCommand.ExecuteAsync(null);
|
||||
|
||||
vault.Keys.ShouldHaveSingleItem().Key.Passphrase.ShouldBeNull("nothing on disk says what it is");
|
||||
|
||||
var reported = import.KeyReport.ShouldHaveSingleItem();
|
||||
|
||||
reported.ShouldContain("passphrase");
|
||||
reported.ShouldContain("id_ed25519");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// A config carried from another machine names keys that are not on this one, and that is the ordinary
|
||||
/// case rather than an error. The host still imports — unbound, exactly as it would have without the
|
||||
/// tick — and the report says which file was missing, because otherwise the only symptom is a host that
|
||||
/// asks for a password on a screen that said it would not.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task AKeyFileThatIsNotThere_LeavesTheHostImportedAndUnbound()
|
||||
{
|
||||
await UnlockedAsync();
|
||||
var vault = shell.Vault!;
|
||||
|
||||
var sshDirectory = KeyedConfigDirectory();
|
||||
|
||||
File.Delete(Path.Combine(sshDirectory, "id_ed25519"));
|
||||
|
||||
var import = new ImportViewModel(vault, new SshConfigLocator(sshDirectory));
|
||||
|
||||
await import.ScanCommand.ExecuteAsync(null);
|
||||
|
||||
import.ImportsKeys = true;
|
||||
|
||||
await import.ImportCommand.ExecuteAsync(null);
|
||||
|
||||
vault.Keys.ShouldBeEmpty();
|
||||
vault.Hosts.ShouldHaveSingleItem().Host.SshKeyId.ShouldBeNull();
|
||||
|
||||
import.KeyReport.ShouldHaveSingleItem().ShouldContain("no such file");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A <c>~/.ssh</c> holding a config that names a key, and the key beside it.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The key path is written into the config as an absolute one, which is what
|
||||
/// <c>SshConfigResolver.ExpandHome</c> produces for a real <c>~/.ssh/id_ed25519</c> — the tests cannot
|
||||
/// use a tilde, since that would resolve against the profile of whoever is running them.
|
||||
/// </remarks>
|
||||
private string KeyedConfigDirectory(string? config = null)
|
||||
{
|
||||
var sshDirectory = Path.Combine(directory, $"ssh-{Guid.CreateVersion7():N}");
|
||||
|
||||
Directory.CreateDirectory(sshDirectory);
|
||||
|
||||
var keyPath = Path.Combine(sshDirectory, "id_ed25519");
|
||||
|
||||
File.WriteAllText(keyPath, PrivateKey("QUJDRA"));
|
||||
File.WriteAllText(keyPath + ".pub", "ssh-ed25519 AAAAC3Nz nobody@example\n");
|
||||
|
||||
var text = config ?? """
|
||||
Host web-01
|
||||
HostName web-01.internal
|
||||
User deploy
|
||||
IdentityFile ~KEY~
|
||||
""";
|
||||
|
||||
File.WriteAllText(
|
||||
Path.Combine(sshDirectory, "config"),
|
||||
text.Replace("~KEY~", keyPath, StringComparison.Ordinal));
|
||||
|
||||
return sshDirectory;
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ImportingWithNoConfigFile_SaysSoRatherThanFailing()
|
||||
{
|
||||
@@ -4047,7 +4271,12 @@ public sealed class ShellFlowTests : IAsyncLifetime
|
||||
var groupId = Guid.CreateVersion7();
|
||||
|
||||
await vault.ImportHostsAsync(
|
||||
[new HostSecret { Label = "prod-db", Hostname = "db.internal", GroupId = groupId }],
|
||||
[
|
||||
new ImportedHostRequest(
|
||||
new HostSecret { Label = "prod-db", Hostname = "db.internal", GroupId = groupId },
|
||||
Key: null,
|
||||
KeyPath: null),
|
||||
],
|
||||
Token);
|
||||
|
||||
vault.Groups.ShouldBeEmpty("nothing in this keychain answers to that id");
|
||||
|
||||
@@ -0,0 +1,224 @@
|
||||
using System.Buffers.Binary;
|
||||
using System.Text;
|
||||
|
||||
namespace DodoSSH.Client.Import.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Reading the private keys an <c>ssh_config</c> names, and telling the protected ones apart.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The encryption check is what most of this is about, and it earns the attention: a key imported without a
|
||||
/// passphrase it needs is a host that fails at connect time with a message about a malformed key, and
|
||||
/// nothing in the interface would say which of the fifteen files it was. So the three formats are each
|
||||
/// pinned, and so is the one that answers by decoding rather than by reading a header.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The rest is refusals. Every one of them is a row on a screen rather than an exception — a config carried
|
||||
/// from another machine names keys that are not on this one, which is the ordinary case, and one bad path
|
||||
/// must not stop the other thirty-nine entries being imported.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class SshIdentityFileTests
|
||||
{
|
||||
[Fact]
|
||||
public void AKeyBesideItsPublicHalf_ComesBackWithBoth()
|
||||
{
|
||||
using var home = new TemporaryDirectory();
|
||||
|
||||
var path = home.Write("id_ed25519", OpenSshKey("none"));
|
||||
home.Write("id_ed25519.pub", "ssh-ed25519 AAAAC3Nz nobody@example\n");
|
||||
|
||||
var read = SshIdentityFiles.Read(path, home.Path);
|
||||
|
||||
read.WasRead.ShouldBeTrue(read.Failure);
|
||||
read.PublicKey.ShouldBe("ssh-ed25519 AAAAC3Nz nobody@example");
|
||||
read.IsEncrypted.ShouldBeFalse();
|
||||
read.Failure.ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// A missing <c>.pub</c> costs the public half and nothing else. It is optional on
|
||||
/// <c>SshKeySecret</c> for the reason written there — it cannot be derived without parsing every format
|
||||
/// the key is stored verbatim in — so its absence is not a failure and must not read as one.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public void AKeyWithNoPublicHalf_IsStillRead()
|
||||
{
|
||||
using var home = new TemporaryDirectory();
|
||||
|
||||
var read = SshIdentityFiles.Read(home.Write("id_ed25519", OpenSshKey("none")), home.Path);
|
||||
|
||||
read.WasRead.ShouldBeTrue(read.Failure);
|
||||
read.PublicKey.ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// A relative <c>IdentityFile</c> resolves against <c>~/.ssh</c>, which is OpenSSH's own rule and is why
|
||||
/// this call takes the directory at all. <c>SshConfigResolver</c> expands a tilde and leaves everything
|
||||
/// else alone, so a bare <c>id_rsa</c> arrives here exactly as it was written.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public void ARelativePath_ResolvesAgainstTheSshDirectory()
|
||||
{
|
||||
using var home = new TemporaryDirectory();
|
||||
|
||||
home.Write("id_rsa", OpenSshKey("none"));
|
||||
|
||||
var read = SshIdentityFiles.Read("id_rsa", home.Path);
|
||||
|
||||
read.WasRead.ShouldBeTrue(read.Failure);
|
||||
read.Path.ShouldBe(Path.Combine(home.Path, "id_rsa"));
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// ◆ The one that decodes rather than reading a header. OpenSSH's own container says nothing about
|
||||
/// encryption in its armour — the cipher name is the first field inside the base64 — and it is what
|
||||
/// <c>ssh-keygen</c> has written by default for years, so it is the case that actually turns up.
|
||||
/// </remarks>
|
||||
[Theory]
|
||||
[InlineData("none", false)]
|
||||
[InlineData("aes256-ctr", true)]
|
||||
[InlineData("aes256-gcm@openssh.com", true)]
|
||||
public void AnOpenSshKey_IsJudgedByTheCipherInsideIt(string cipher, bool encrypted)
|
||||
{
|
||||
using var home = new TemporaryDirectory();
|
||||
|
||||
var read = SshIdentityFiles.Read(home.Write("id_ed25519", OpenSshKey(cipher)), home.Path);
|
||||
|
||||
read.WasRead.ShouldBeTrue(read.Failure);
|
||||
read.IsEncrypted.ShouldBe(encrypted);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// The two formats that do say so in the armour. Classic PEM carries a <c>Proc-Type</c> line above the
|
||||
/// base64; PKCS#8 puts it in the header outright.
|
||||
/// </remarks>
|
||||
[Theory]
|
||||
[InlineData("-----BEGIN RSA PRIVATE KEY-----\nProc-Type: 4,ENCRYPTED\nDEK-Info: AES-128-CBC,00\n\nQUJD\n-----END RSA PRIVATE KEY-----\n")]
|
||||
[InlineData("-----BEGIN ENCRYPTED PRIVATE KEY-----\nQUJD\n-----END ENCRYPTED PRIVATE KEY-----\n")]
|
||||
public void AnArmourThatSaysItIsEncrypted_IsTakenAtItsWord(string material)
|
||||
{
|
||||
using var home = new TemporaryDirectory();
|
||||
|
||||
var read = SshIdentityFiles.Read(home.Write("id_rsa", material), home.Path);
|
||||
|
||||
read.WasRead.ShouldBeTrue(read.Failure);
|
||||
read.IsEncrypted.ShouldBeTrue();
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// <c>ssh</c> tolerates an <c>IdentityFile</c> naming the <c>.pub</c> — it looks for the private half
|
||||
/// beside it — so people write it. Importing the public one would produce a keychain entry that looks
|
||||
/// fine and fails at connect time saying nothing about which of two files whose names differ by four
|
||||
/// characters was chosen, which is the mistake <c>SshKeySecret.TryValidate</c> exists to catch.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public void ThePublicHalfNamedAsTheIdentityFile_IsRefusedByName()
|
||||
{
|
||||
using var home = new TemporaryDirectory();
|
||||
|
||||
var path = home.Write("id_ed25519.pub", "ssh-ed25519 AAAAC3Nz nobody@example\n");
|
||||
|
||||
var read = SshIdentityFiles.Read(path, home.Path);
|
||||
|
||||
read.WasRead.ShouldBeFalse();
|
||||
read.Failure.ShouldContain("public half");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AFileThatIsNotAKeyAtAll_IsRefused()
|
||||
{
|
||||
using var home = new TemporaryDirectory();
|
||||
|
||||
var read = SshIdentityFiles.Read(home.Write("notes.txt", "remember to rotate this"), home.Path);
|
||||
|
||||
read.WasRead.ShouldBeFalse();
|
||||
read.Failure.ShouldContain("private key");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// The ordinary case for a config carried between machines, and the reason none of this throws: the
|
||||
/// path is still named, so the import screen can say which entry lost its key rather than reporting
|
||||
/// that something somewhere went wrong.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public void APathThatIsNotThere_NamesItselfInTheRefusal()
|
||||
{
|
||||
using var home = new TemporaryDirectory();
|
||||
|
||||
var read = SshIdentityFiles.Read("id_absent", home.Path);
|
||||
|
||||
read.WasRead.ShouldBeFalse();
|
||||
read.Failure.ShouldContain("no such file");
|
||||
read.Path.ShouldContain("id_absent");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An armoured OpenSSH v1 private key whose cipher field says what is asked for.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Padded past 66 bytes because the reader decodes the first 88 base64 characters and needs the magic
|
||||
/// and the first field inside them — a container shorter than that is not a shape <c>ssh-keygen</c>
|
||||
/// produces, and treating one as unencrypted is the safe direction anyway.
|
||||
/// </remarks>
|
||||
private static string OpenSshKey(string cipher)
|
||||
{
|
||||
var body = new List<byte>();
|
||||
|
||||
body.AddRange("openssh-key-v1\0"u8);
|
||||
|
||||
foreach (var field in new[] { cipher, "none", string.Empty })
|
||||
{
|
||||
var length = new byte[4];
|
||||
BinaryPrimitives.WriteUInt32BigEndian(length, (uint)field.Length);
|
||||
|
||||
body.AddRange(length);
|
||||
body.AddRange(Encoding.ASCII.GetBytes(field));
|
||||
}
|
||||
|
||||
while (body.Count < 96)
|
||||
{
|
||||
body.Add(0x41);
|
||||
}
|
||||
|
||||
var armour = Convert.ToBase64String(body.ToArray());
|
||||
|
||||
return $"-----BEGIN OPENSSH PRIVATE KEY-----\n{armour}\n-----END OPENSSH PRIVATE KEY-----\n";
|
||||
}
|
||||
|
||||
/// <summary>A directory standing in for <c>~/.ssh</c>, removed when the test finishes.</summary>
|
||||
private sealed class TemporaryDirectory : IDisposable
|
||||
{
|
||||
internal TemporaryDirectory()
|
||||
{
|
||||
Path = System.IO.Path.Combine(
|
||||
System.IO.Path.GetTempPath(), $"dodossh-identity-{Guid.CreateVersion7():N}");
|
||||
|
||||
Directory.CreateDirectory(Path);
|
||||
}
|
||||
|
||||
internal string Path { get; }
|
||||
|
||||
internal string Write(string name, string content)
|
||||
{
|
||||
var path = System.IO.Path.Combine(Path, name);
|
||||
|
||||
File.WriteAllText(path, content);
|
||||
|
||||
return path;
|
||||
}
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
try
|
||||
{
|
||||
Directory.Delete(Path, recursive: true);
|
||||
}
|
||||
catch (IOException)
|
||||
{
|
||||
// A test's leftovers in the system temp directory are not worth failing a run over.
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user