Replace internal BouncyCastle with NuGet package #1370
Conversation
|
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. |
for some reason d24fe20 introduced its own names here which (as far as I can see) were never part of the original BouncyCastle. Switched to the correct names.
|
tests fixed with b7f6cd4 |
|
How about leveraging BCL's |
|
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. |
|
Out of curiosity what is the lowest supported version of the Windows. Windows 2012 is EOL
|
|
@mus65 I created a separate PR #1371. Might bring a little bit conflicts with your PR but it should be straightforward to resolve and should not block this PR. |
|
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?
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. |
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). |
|
Just curious to see latest CI |
|
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. |
|
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. |
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.
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 bcgit/bc-csharp#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. |
|
Updated to BouncyCastle 2.4.0 which already includes @Rob-Hague's trimming fixes. 😄 |
CA1724: The type name Zlib conflicts in whole or in part with the namespace name 'Org.BouncyCastle.Utilities.Zlib'. Change either name to eliminate the conflict. We can't change either, so suppress the warning.
|
I think we should take this for this release. I will come back to it after some other stuff. I want to a) experiment with replacing the X25519 Na.Cl code as well; and b) check the size of an AOT app, perhaps with some check in CI |
|
IMO, we should use BouncyCastle Nuget and mark all old releases as unsafe. Let's look at how many vulnerabilities they resolved in old versions. Then we can implement #1416 PR using this library. |
|
I think we can get rid of Chaos.NaCI now. |
|
Chaos.NaCl can be removed if merge #1416, #1447 and #1448. @WojciechNagorski @Rob-Hague |
|
Nice, I'll review in the next few days |
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 .