Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elliptic Curves #496

Merged
merged 8 commits into from
Jan 11, 2020
Merged

Elliptic Curves #496

merged 8 commits into from
Jan 11, 2020

Conversation

darinkes
Copy link
Collaborator

@darinkes darinkes commented Dec 2, 2018

Latest approach to implement Elliptic Curves Support in SSH.NET.

  • ED25519 and Curve25519 for KEX, Hostkey and Private Keys
  • ECDH
    • Minimal Import (just the needed parts) of http://www.bouncycastle.org/csharp/
    • MIT like License
    • Microsoft's ECDiffieHellmanCng doesn't work... :/
    • Timing Safe is not important for KEX, since keys don't live long. Couldn't find any verification that it resists timing attacks
  • ECDSA for Hostkey and Private Keys
    • System.Security.Cryptography.ECDsaCng
    • Timing Safe (hopefully! ;))

Performance-Comparison Key-Exchange:

  • Code
var sw = Stopwatch.StartNew();
using (var client = new SshClient(coninfo))
{
     client.Connect();
     Console.WriteLine(client.RunCommand("hostname").Result);
     client.Disconnect();
}
sw.Stop();
Console.WriteLine("Time taken: {0}ms", sw.Elapsed.TotalMilliseconds);
  • diffie-hellman-group-exchange-sha256
    Time taken: 2365,9079ms
  • curve25519-sha256
    Time taken: 1169,3554ms
  • ecdh-sha2-nistp256
    Time taken: 1053,3225ms

@drieseng
Copy link
Member

drieseng commented Dec 2, 2018

@darinkes Is this the minimal set of sources we need to "import"?

@Kim-SSi
Copy link

Kim-SSi commented Dec 3, 2018

It is all in the src/Renci.SshNet/Security/BouncyCastle folder. See:
https://github.com/darinkes/SSH.NET-1/tree/elliptic/src/Renci.SshNet/Security/BouncyCastle/src

If this is the approach taken, please change the namespace to avoid collisions with the BouncyCastle assemblies or make it all internal - provided that is possible.
namespace Org.BouncyCastle...
to or similar e.g:
namespace Renci.SshNet.Org.BouncyCastle...

@darinkes
Copy link
Collaborator Author

darinkes commented Dec 3, 2018

@drieseng
I think there can be done more. E.g. the in the ASN.1 folder of BouncyCastle. And remove stuff marked as obsolete.

Edit:
And of course reenable build warnings/errors again.

@darinkes
Copy link
Collaborator Author

darinkes commented Dec 3, 2018

@drieseng
I think there can be done more. E.g. the in the ASN.1 folder of BouncyCastle. And remove stuff marked as obsolete.

Okay, was wrong here twice :)
ASN.1 is a chain of dependencies and BouncyCastle relies internally on stuff they marked as obsolete.

Edit:
And of course reenable build warnings/errors again.

Done! All now in an internal namespace, nothing gets exposed.

Whats the best/easiest way to enable the Build for all the other projects?

@darinkes darinkes force-pushed the elliptic branch 6 times, most recently from fcd5540 to 9b76b84 Compare December 3, 2018 11:12
@darinkes
Copy link
Collaborator Author

darinkes commented Dec 3, 2018

Ok, the last Build Fails are not my fault :)

This Test seems to flipper:
Renci.SshNet.Tests.Classes.SessionTest_Connected_ServerSendsBadPacket.ReceiveOnServerSocketShouldReturnZero()
First all passed, then failed on .NET 3.5 and now .NET.
And all I changed was the Commit Messages to trigger an rebuild.

@darinkes darinkes force-pushed the elliptic branch 4 times, most recently from cb2a922 to 6fecae9 Compare December 4, 2018 20:51
@darinkes
Copy link
Collaborator Author

darinkes commented Dec 4, 2018

Spend some time on cleaning up all the commits and (hopefully) split them in reasonable/reviewable parts.

@darinkes darinkes force-pushed the elliptic branch 2 times, most recently from 3d969da to 7a13b54 Compare December 5, 2018 11:00
@darinkes
Copy link
Collaborator Author

darinkes commented Dec 5, 2018

@drieseng
I think there can be done more. E.g. the in the ASN.1 folder of BouncyCastle. And remove stuff marked as obsolete.

Okay, was wrong here twice :)
ASN.1 is a chain of dependencies and BouncyCastle relies internally on stuff they marked as obsolete.

Got rid most of the stuff in the asn.1 folder with some minor adjustments in the sources. Over 10k Lines less than before.

@drieseng
Copy link
Member

drieseng commented Dec 5, 2018

Please also add a THIRD-PARTY-NOTICES.TXT to the repo, and include the BouncyCastle and Chaos.NaCl licenses. You can find inspiration here.

src/Renci.SshNet.NETCore/Renci.SshNet.NETCore.csproj Outdated Show resolved Hide resolved
src/Renci.SshNet.NETCore/Renci.SshNet.NETCore.csproj Outdated Show resolved Hide resolved
src/Renci.SshNet/Common/DerData.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/Common/DerData.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/Common/DerData.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/PrivateKeyFile.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/Security/Cryptography/Bcrypt.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/Common/Extensions.cs Show resolved Hide resolved
@@ -92,7 +92,7 @@ public BigInteger[] Keys
for (var i = 0; i < _keys.Count; i++)
{
var key = _keys[i];
keys[i] = key.ToBigInteger();
keys[i] = BigInteger.SshFormatBignum2(key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to explain why this is necessary? Would this break other algorithms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a Key comes with a leading "FF", the current ToBigInteger() would drop this "FF" and we have invalid data. That gets prevented by appending an "00" before the "FF".

Made a lot of testruns with DSA and RSA and had no Problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that covers this?
Haven't had time to check if this is trivial.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bhalbright
Copy link

Please also add a THIRD-PARTY-NOTICES.TXT to the repo, and include the BouncyCastle and Chaos.NaCl licenses. You can find inspiration here.

For what its worth, the BCrypt that I added came from here: https://bitbucket.org/vadim/bcrypt.net/src/9016bf211b5bc7406e01f2f99fe5b868b1e35238/BCrypt.Net/BCrypt.cs?at=default&fileviewer=file-view-default, with a few additions for the kdf stuff. Not sure if that needs to go in the 3rd party file mentioned, but wanted to point it out

@darinkes darinkes force-pushed the elliptic branch 2 times, most recently from 9d07604 to b367b1b Compare December 6, 2018 07:50
@darinkes
Copy link
Collaborator Author

darinkes commented Dec 9, 2018

Merged the Linux Support to "Add Support for ECDSA Host- and Private-Keys", since this makes automatic real life testing much much easier.

@gargml
Copy link

gargml commented Feb 6, 2019

Hi Guys,
I must share that the requirement of supporting Elliptic Curves in SSH.NET, is raising from day to day,
Hence, will it be possible to share if you have any estimations when will such support will be added to the SSH.NET library ?
Is there some beta version i can test ?
Please share

@MikeBairdRocks
Copy link

@drieseng How can we get this out the door? Any help needed with the review? Seems there is a need for this by a few people. I know 30k line diff is a little overwhelming. 🤔

@darinkes
Copy link
Collaborator Author

@MikeBairdRocks tests and comments are always welcome and I'm sure they help to get this in.

@kv9y
Copy link

kv9y commented Jul 12, 2019

Is there any way I could get a version of SSH.NET with KEX ecdh-sha2-nistp256 from this PR, enabled? I would be more than happy to test, comment, what-have-you. Just point me in the correct direction.

@Psilax
Copy link

Psilax commented Aug 20, 2019

Another month further, any more feedback on the planning to release this feature @drieseng and what is required for this?

@kv9y
Copy link

kv9y commented Sep 3, 2019

Is there any update on this PR?

Copy link

@BorisAmelyan BorisAmelyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we release OPENSSH?

@darinkes
Copy link
Collaborator Author

darinkes commented Dec 4, 2019

🎉🎁🎂

@lifeincha0s
Copy link

lifeincha0s commented Jan 10, 2020

I have been working through the code to update crypto to be FIPS compliant. I added diffie-hellman-group14-sha256, diffie-hellman-group16-sha512, and diffie-hellman-group18-sha512. I wrote ecdh-sha2-nistp256 as research for my grad crypto class, so I implemented it without using any Microsoft or 3rd party libraries. I chose that option because using .NET crypto required adding too much overly complicated code for it to be usable for public and shared key creation, as well as generating signatures for kex. 3rd party libraries have so much overhead that I would rather implement my own functionality. I am also removing antiquated code to stay up with RFC standards (i.e. anything from SSHv1)

@darkoperator
Copy link

darkoperator commented Jan 10, 2020 via email

@drieseng
Copy link
Member

I'm reviewing this PR right now.
I hope to have it merged this weekend.

@drieseng
Copy link
Member

drieseng commented Jan 11, 2020

@darinkes Can you, for all of the third party sources you added, indicate where you obtained the sources from (source distribution, git(hub), ...), and what exact version of these sources? Thanks for this great contribution!

@drieseng drieseng merged commit 546e2b9 into sshnet:develop Jan 11, 2020
@BorisAmelyan
Copy link

when will the new version be available for upgrade on NugetPackage manager?

@drieseng
Copy link
Member

I've lost my VMs for building SSH.NET on all supported target frameworks.
I hope to be able to complete this in the next two weeks, but no promises.

@BorisAmelyan
Copy link

Thank you!

@darinkes
Copy link
Collaborator Author

@darinkes Can you, for all of the third party sources you added, indicate where you obtained the sources from (source distribution, git(hub), ...), and what exact version of these sources? Thanks for this great contribution!

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.