Update BouncyCastle to new official package

[WouterTinus]

Is your feature request related to a problem? Please describe.
Currently MailKit depends on Portable.BouncyCastle which is a (semi?)unofficial port of BouncyCastle to .NET Standard. Recently, the BouncyCastle project have released their own package with support for .NET Standard (and .NET6). See the announcement and the package BouncyCastle.Cryptography.

For my project I'd like to switch to the new official BouncyCastle.Cryptography package. I think that version will prove to be better maintained and adhere to higher standards of security. However my solution fails to build if I try to switch, because then two assemblies declare the same types in the same namespaces. One is my direct dependency on BouncyCastle.Cryptography and the second the indirect dependency on Portable.BouncyCastle that I take through MailKit.

Describe the solution you'd like
For MailKit to also update to the official BouncyCastle.Cryptography package.

Describe alternatives you've considered
It might be possible to use assembly redirection to fix the compiler error, but that seems risky from a stability perspective because it's a major new release with more than a year worth of fixes.

Additional context
N/A

[jstedfast]

Yes, I plan to make the switch.

[WouterTinus]

I followed your progress and noted that the changes were reverted in 45d331f.

Per the revert commit message:

BouncyCastle.Cryptography 2.0.0 is causing too many unitt tests to fail and I don't know why.

1. Decompressing S/MIME-compressed data fails with an exception statring that the compression
   algorithm used is unknown/invalid, but the compression algorithm used was ZLib. WTF?
2. A bunch of S/MIME tests are failing due to "Org.BouncyCastle.Asn1.Asn1ParsingException:
   long form definite-length more than 31 bits" when verifying/decrypting. No idea how to
   fix this.
3. Pkcs12Store.Load(Stream input, Char[] password) fails with an NRE which doesn't make
   sense because the input and password arguments are not null, so it must be a bug in
   BouncyCastle that is accessing, perhaps, an uninitialized member variable.

A couple of weeks later the BouncyCastle project have released version 2.1.1. The changelog doesn't necessarily mention those specific issues, but I wonder if it's worth everybodies time to reopen this issue to track potential progress for a retry, and perhaps get in touch with the developers of that project at https://github.com/bcgit/bc-csharp/issues to help identify and fix the apparent regressions. Since a commit with failing unit tests exists, I guess it wouldn't take to much effort to point them towards some of the specifics.

[jstedfast]

Fear not, I want to move to official BouncyCastle 2.x - I just need to find enough free time to work on it.

It has also become apparent that I will likely want to bump MimeKit's major version number to 4 when I do because of the major changes in BouncyCastle.

but I wonder if it's worth everybodies time to reopen this issue

Yes, absolutely. I forgot that there was an issue regarding this (I thought there was just the Pull Request).

and perhaps get in touch with the developers of that project at https://github.com/bcgit/bc-csharp/issues to help identify and fix the apparent regressions

Before doing that, someone (probably me) needs to take the time to write a minimal test case of each of the issues or we won't get very far with getting help most likely.

[jstedfast]

Submitted one of the issues I am hitting here: bcgit/bc-csharp#423

[jstedfast]

I found and fixed the Pkcs12Store.Load() issue. That code was getting called on an empty stream which is why it failed.

[jstedfast]

Looks like the bugs are in BouncyCastle's netstandard2.1/net6.0 codepaths. When I build the unit tests against net48 instead of net6.0 and run them, they all pass.

[jstedfast]

Turns out there was a bug in MimeKit where it would return a shared buffer to a pool and then continue using it (and then return it again which probably resulted in the buffer pool renting the same buffer out to multiple locations at the same time later).

That is what was causing memory corruption in the decompression logic and the other S/MIME unit tests.

Everything in the bouncycastle2.0 branch now works and all of the unit tests pass.

(Thanks to Peter Dettman for digging into this issue and coming up with the theory that there was probably a bug in MimeKit's usage of array pools somewhere).

[WouterTinus]

Nice find, thanks @jstedfast and @peterdettman!

[jstedfast]

The patch has now landed in master which means you can start testing it via the MyGet packages linked from the README (and/or the main GitHub project page)

[jstedfast]

I think when I make a release with this, it'll be versioned 4.0.0 (since I suspect people will get upset if I call it 3.7.0 due to the change in BouncyCastle dependency).

I need to do some work on MailKit to make sure that its code that uses BouncyCastle is also update to work properly

[zabulus]

I think you haven't updated .nuspec file, as nupkg at github actions still has Portable.BouncyCastle as dependency. Same at myget feed.

[jstedfast]

Thanks, I just fixed that so it should be showing up in MyGet probably in the next 30m or so

[jstedfast]

MimeKit v4.0 has been released with support for the new BouncyCastle