search

sshnet / SSH.NET

SSH.NET is a Secure Shell (SSH) library for .NET, optimized for parallelism.
http://sshnet.github.io/SSH.NET/
MIT License
3.88k stars 917 forks source link

Replace internal BouncyCastle with NuGet package #1370

Open mus65 opened 2 months ago

mus65 commented 2 months ago

Contributes to #1271 .

This was as simple as adding the NuGet package, deleting the old code and replacing the namespaces. It's just a matter of whether the tests pass and you actually want to do this. I don't think there was a clear decision in #1271 .

mus65 commented 2 months ago

Guess it wasn't this simple after all, a lot of tests fail with a NullReferenceException. I'm gonna take a look within the next few days. Converted to draft.

mus65 commented 2 months ago

tests fixed with b7f6cd4b4f86a2c8c87ec0d8fee2a4f354683f58

scott-xu commented 2 months ago

How about leveraging BCL's ECDiffieHellman.DeriveRawSecretAgreement for .NET 8 onward? See https://github.com/dotnet/runtime/issues/71613

mus65 commented 2 months ago

Thanks, that's good to know. Unfortunately this API is not supported on Windows Server 2012, so even net8.0 builds would still need the BouncyCastle reference.

Not saying this shouldn't be done. But since this wouldn't reduce the dependency to BouncyCastle it imho shouldn't block this PR and could be done separately.

darkoperator commented 2 months ago

Out of curiosity what is the lowest supported version of the Windows. Windows 2012 is EOL

scott-xu commented 2 months ago

@mus65 I created a separate PR https://github.com/sshnet/SSH.NET/pull/1371. Might bring a little bit conflicts with your PR but it should be straightforward to resolve and should not block this PR. @darkoperator See this please: https://github.com/sshnet/SSH.NET/pull/1371#issuecomment-2041099786

Rob-Hague commented 2 months ago

Thanks. Indeed there was no decision on this, and I don't have a preference either way (which probably means I'd lean towards not doing anything). I think I'll leave this one for a while for others to chime in.

One thing: this library has a similar internal dependency on some Chaos.NaCl code for ed25519. Perhaps as part of this change (if it were to be merged), that code could be removed as well and replaced with some additional usage of BouncyCastle. Would you be interested in investigating that?

Guess it wasn't this simple after all

FYI, running the integration tests locally is very simple - just install Docker Desktop and it should "just work". It's a lot faster than relying on CI.

mus65 commented 2 months ago

One thing: this library has a similar internal dependency on some Chaos.NaCl code for ed25519. Perhaps as part of this change (if it were to be merged), that code could be removed as well and replaced with some additional usage of BouncyCastle. Would you be interested in investigating that?

I actually had a look at that, but unfortunately I'm not familiar enough with cryptography to do this.

Side note: the internal BouncyCastle doesn't support Ed25519 yet, so in order to get rid of Chaos.NaCI, either this PR needs to be merged first or the internal BouncyCastle needs to be updated. I also found this BouncyCastle issue about some implementation difference with NaCl (I don't know if this is relevant for SSH.NET).

Rob-Hague commented 2 months ago

Just curious to see latest CI

lifeincha0s commented 1 month ago

Chiming in... Adding BouncyCastle as a NuGet package would add all of the lib features, or am I wrong about that? Wouldn't that be a bit excessive since only a very limited subset is actually implemented and NET has been supporting more and more crypto natively.

mus65 commented 1 month ago

Updated to BouncyCastle 2.3.1 which fixed multiple CVEs. These CVEs are obviously unfixed in the internal implementation. I can't tell whether these actually affect SSH.NET, but either way this imho would be another reason in favor of the NuGet package.

If one of the CVEs affects SSH.NET, fixing it would require updating the internal implementation and making a new release. With the NuGet package, any application using SSH.NET could simply update to the newer BouncyCastle version to get the security fixes.

Rob-Hague commented 1 month ago

Wouldn't that be a bit excessive since only a very limited subset is actually implemented

Right, if we are only using it for ECDH it is arguably not worth it. But if we can also use it for ed25519, bcrypt, standard DH(?) and Chachapoly(?) then there is more of a case for it.

I can't tell whether these actually affect SSH.NET, but either way this imho would be another reason in favor of the NuGet package.

They don't look like they affect our usage but your point stands either way.

The hesitation stems from that the BC assembly does not play well with trimming. I've opened https://github.com/bcgit/bc-csharp/pull/534 to partly address that and a full fix is possible. I'd like to see if that gets anywhere. If it does then we will be able to say if you care about the extra size then you should trim your app, and if you don't care then you don't care. Right now a trimmed BC assembly is still 4MB.

If the BC trimming changes don't get anywhere then I guess we just bite the bullet.

mus65 commented 4 weeks ago

Updated to BouncyCastle 2.4.0 which already includes @Rob-Hague's trimming fixes. :smile: