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.

Sign up for GitHub

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

Jump to bottom

Use BCL ECDiffieHellman for KeyExchange instead of BouncyCastle (.NET 8.0 onward only) #1371

Merged
merged 33 commits into from
Jul 31, 2024
Merged

Use BCL ECDiffieHellman for KeyExchange instead of BouncyCastle (.NET 8.0 onward only) #1371

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
901fd5a
Use BCL ECDiffieHellman for KeyExchange (.NET 8.0 onward only)
scott-xu Apr 6, 2024
f78488c
Merge branch 'develop' into ecdh-bcl
scott-xu Apr 6, 2024
4b8be40
Add back an empty line
scott-xu Apr 6, 2024
8e6d079
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu Apr 6, 2024
61e44a4
Remove the BouncyCastle dependency when target .NET 8.0 onward.
scott-xu Apr 7, 2024
767e692
Merge branch 'develop' into ecdh-bcl
scott-xu Apr 7, 2024
4628a73
Run KeyExchangeAlgorithmTests for .NET 6.0
scott-xu Apr 9, 2024
23c4ac2
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu Apr 9, 2024
23a1dd3
Build Renci.SshNet.IntegrationTests.csproj for net6.0
scott-xu Apr 9, 2024
182f586
Update filter
scott-xu Apr 9, 2024
fed031b
Merge branch 'develop' into ecdh-bcl
scott-xu Apr 18, 2024
cfd950f
Merge branch 'develop' into ecdh-bcl
scott-xu Apr 25, 2024
8392edb
Merge branch 'develop' into ecdh-bcl
scott-xu May 4, 2024
8b42e59
Merge branch 'develop' of https://github.com/scott-xu/SSH.NET into ec…
scott-xu May 23, 2024
b35ffdf
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu May 23, 2024
4b73a96
Merge branch 'develop' into ecdh-bcl
scott-xu Jun 9, 2024
5502918
Add back BouncyCastle as fallback
scott-xu Jun 12, 2024
f76ffeb
Merge branch 'develop' into ecdh-bcl
scott-xu Jun 13, 2024
a1b00e3
Add back the missing `SendMessage`
scott-xu Jun 13, 2024
6ed28ae
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu Jun 13, 2024
d754e93
Merge branch 'develop' into ecdh-bcl
scott-xu Jun 16, 2024
08c9594
Merge branch 'develop' of https://github.com/scott-xu/SSH.NET into ec…
scott-xu Jul 17, 2024
4495c57
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu Jul 17, 2024
bd093c7
Merge branch 'develop' into ecdh-bcl
scott-xu Jul 23, 2024
57bf19d
Merge branch 'develop' into ecdh-bcl
WojciechNagorski Jul 25, 2024
13dd7e1
Run ECDH KEX integration tests under .NET48
scott-xu Jul 25, 2024
9fa0c14
Merge branch 'develop' into ecdh-bcl
scott-xu Jul 25, 2024
344b744
Merge branch 'develop' into ecdh-bcl
scott-xu Jul 27, 2024
385e087
Use SshNamedCurves instead of SecNamedCurves for BouncyCastle.
scott-xu Jul 28, 2024
387e6da
typo
scott-xu Jul 28, 2024
73c9446
Fix build
scott-xu Jul 29, 2024
db0a98e
Use System.Security.Cryptography namespace if NET8_0_OR_GREATER;
scott-xu Jul 29, 2024
2441f77
Separate BCL and BouncyCastle implementation
scott-xu Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Renci.SshNet/Security/KeyExchangeECDH.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
using System;

using Org.BouncyCastle.Asn1.Sec;
using Org.BouncyCastle.Crypto.Agreement;
using Org.BouncyCastle.Crypto.Generators;
using Org.BouncyCastle.Crypto.Parameters;
using Org.BouncyCastle.Crypto.Utilities;
using Org.BouncyCastle.Math.EC;

using Renci.SshNet.Abstractions;
Expand Down Expand Up @@ -55,7 +55,7 @@ public override void Start(Session session, KeyExchangeInitMessage message, bool
return;
}
#endif
var curveParameter = SecNamedCurves.GetByName(CurveName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell, going through SshNamedCurves causes a bunch of extra lookup and static initialization compared to SecNamedCurves. Is there any reason to use it?

It's a shame we can't just directly refer to the static BC object when we know the curve we want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess SshNamedCurves is designed for libraries like SSH.NET to consume?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, but I don't see why we would. If I run this program in a loop:

public static void Main()
{
    var s = Stopwatch.StartNew();

    //if (!SecNamedCurves.GetByName("secp256r1").G.IsInfinity)
    if (!SshNamedCurves.GetByName("nistp256").G.IsInfinity)
    {
        Console.WriteLine(s.ElapsedTicks);
    }
}
dotnet build -c Release; do { dotnet run -c Release --no-build } while ($true)

The result is about 28.3±0.7ms vs 29.2±0.8ms. Both extremely high (also includes first JIT of relevant BouncyCastle methods), but 1ms is 1ms

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would you mind trying SecNamedCurves.GetByOid(SecObjectIdentifiers.SecP256r1)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't try it but I'm pretty sure it would be the same as SecNamedCurves.GetByName. The problem with SshNamedCurves is the amount of static constructors that it forces to run

var curveParameter = SshNamedCurves.GetByName(CurveName);
_domainParameters = new ECDomainParameters(curveParameter.Curve,
curveParameter.G,
curveParameter.N,
Expand Down
2 changes: 1 addition & 1 deletion src/Renci.SshNet/Security/KeyExchangeECDH256.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public override string Name
/// </summary>
protected override string CurveName
{
get { return "secp256r1"; }
get { return "nistp256"; }
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Renci.SshNet/Security/KeyExchangeECDH384.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public override string Name
/// </summary>
protected override string CurveName
{
get { return "secp384r1"; }
get { return "nistp384"; }
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Renci.SshNet/Security/KeyExchangeECDH521.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public override string Name
/// </summary>
protected override string CurveName
{
get { return "secp521r1"; }
get { return "nistp384"; }
}

/// <summary>
Expand Down