Skip to content

Conversation

0xced
Copy link

@0xced 0xced commented Nov 15, 2021

  • Include using directives to make it clear that System.Linq and Renci.SshNet are required
  • Remove unnecessary new byte[] in expectedFingerPrint declaration
  • Use discard (i.e. _) instead of unused sender argument (C# 7.0)
  • Use Enumerable.SequenceEqual instead of manually comparing byte arrays
  • using var client without braces (C# 8.0 feature)

@A9G-Data-Droid
Copy link

I would like a note about how to get the key into byte[] format.

Related reading: https://winscp.net/eng/docs/faq_hostkey

That only works if you have the ability to get the public key from a known good source.

I like to store it in a Base64 string. So I'm pulling it from a config file like this:

byte[] expectedFingerPrint = Convert.FromBase64String(ServerFingerPrint);

If you want to pull the fingerprint from your code so you can accept the server fingerprint you need something like this:

client.HostKeyReceived += (_, e) =>
{
    ServerFingerPrint = Convert.ToBase64String(e.FingerPrint);
    e.CanTrust = expectedFingerPrint.SequenceEqual(e.FingerPrint);
};

Then when .CanTrust is false you can give the user the option of saving this fingerprint, which is a normal workflow in most SSH\SFTP clients.

This at least serves to notify you when the fingerprint has changed, which is better than nothing.

@WojciechNagorski
Copy link
Collaborator

Old PR, closing for now.

@0xced
Copy link
Author

0xced commented Aug 25, 2023

Sure it's old but I think it's still relevant, isn't it?

If there are conflicts with the current state of the develop branch I'd be happy to resolve them.

I'd understand if you don't want to merge this because you'd prefer to keep the sample code compatible with an older C# version or maybe for another reason that I haven't thought about. But closing simply because the pull request old is really disappointing and will likely discourage me to further contribute to this project. 😞

@WojciechNagorski
Copy link
Collaborator

@0xced regarding this comment #897 (comment)

the check should look like below, right?

client.HostKeyReceived += (_, e) =>
{
    ServerFingerPrint = Convert.ToBase64String(e.FingerPrint);
    e.CanTrust = expectedFingerPrint.SequenceEqual(e.FingerPrint);
};

If you prepare a good PR, I will definitely merge it.

@0xced
Copy link
Author

0xced commented Aug 25, 2023

Well, Adam said

I like to store it in a Base64 string.

Personally, I store them in this format: 97:70:33:82:fd:29:3a:73:39:af:6a:07:ad:f8:80:49 because it's an MD5 hash (and that's how Microsoft displays it in their Azure DevOps SSH - Public Keys page):

Azure DevOps Server Fingerprints (both MD5 and SHA256)

using System.Globalization;
using System.Linq;
using Renci.SshNet;

const string expectedFingerPrint = "97:70:33:82:fd:29:3a:73:39:af:6a:07:ad:f8:80:49";
using var client = new SshClient(host: "ssh.dev.azure.com", port: 22, username: "git", password: "");
client.HostKeyReceived += (_, e) =>
{
    var md5FingerPrint = string.Join(':', e.FingerPrint.Select(x => x.ToString("x2", NumberFormatInfo.InvariantInfo)));
    e.CanTrust = md5FingerPrint == expectedFingerPrint;
};
client.Connect();

It would even be misleading to use base64 since it's an MD5 hash and usually the fingerprint displayed as base64 are SHA256 hashes, see also GitHub's SSH key fingerprints.

I still think my pull request is good as is. 🤷

* Include `using` directives to make it clear that `System.Linq` and `Renci.SshNet` are required
* Remove unnecessary `new byte[]` in `expectedFingerPrint` declaration
* Use [discard](https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/functional/discards) (i.e. `_`) instead unused `sender` argument (C# 7.0)
* Use [Enumerable.SequenceEqual](https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.sequenceequal) instead of manually comparing byte arrays
* `using var client` without braces (C# 8.0 feature)
@A9G-Data-Droid
Copy link

A9G-Data-Droid commented Aug 25, 2023

@0xced That's a great point! In newer versions of OpenSSH, Base64 encoded SHA-256 is the default instead of hexadecimal MD5. This topic is related to #613.

I agree that the way you have shown is industry standard, as OpenSSH also reports this colon separated hex format.

The example you give in this PR is good, better than it was. I like your new example better. I would use BitConverter instead of rolling my own.

using System.Globalization;
using System.Linq;
using Renci.SshNet;

const string expectedFingerPrint = "97:70:33:82:fd:29:3a:73:39:af:6a:07:ad:f8:80:49";
using var client = new SshClient("sftp.foo.com", "guest", "pwd")
client.HostKeyReceived += (_, e) =>
{
    var md5FingerPrint = BitConverter.ToString(e.FingerPrint).Replace("-", ":");
    e.CanTrust = md5FingerPrint == expectedFingerPrint;
};

client.Connect();

@0xced
Copy link
Author

0xced commented Aug 25, 2023

Related: I just submitted #1163 which adds two new (well documented) properties: MD5FingerPrint and SHA256FingerPrint. Inspired by this discussion of course. 😉

And if #1163 gets merged the sample code could be improved even further.

About BitConverter vs string.Join() we should benchmark it (both speed and allocations) before choosing the best implementation.

@WojciechNagorski WojciechNagorski added this to the 2023.0.0 milestone Sep 17, 2023
@WojciechNagorski
Copy link
Collaborator

closed by #1186. Readme has been updated.

@WojciechNagorski
Copy link
Collaborator

Version 2023.0.0 has been published https://www.nuget.org/packages/SSH.NET/2023.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants