bcgit / bc-csharp

BouncyCastle.NET Cryptography Library (Mirror)
https://www.bouncycastle.org/csharp
MIT License
1.67k stars 556 forks source link

Thoughts on code modernization? #50

Closed jarz closed 1 year ago

jarz commented 8 years ago

Some idioms seem outdated (implementing IEnumberable but not IEnumerable being a big one).

Is this just an effect of making the C# and Java code look similar? Would clean-up contributions be accepted?

Thanks!

peterdettman commented 8 years ago

It's partly the Java origins, partly backward compatibility, partly supporting .NET 1.1 (1.8 series is the last release for that). We will be forking off a maintenance branch for 1.8 "shortly", at which point we would welcome modernization patches to the master branch (intended target .NET 4).

clairernovotny commented 8 years ago

@peterdettman is there any point in using .NET 4 as the baseline given that .NET 4-4.5.1 is no longer supported by Microsoft for security updates?

Can the baseline be .NET 4.5.2 (or at least 4.5 from an API/compat standpoint)?

jstedfast commented 8 years ago

FWIW, I'm fine with Bouncy Castle deciding to target >= .NET 4.5, but here are a few thoughts:

  1. There's no sense in targeting a .NET platform version higher than what you need API-wise.
  2. .NET 4.0 gives you everything you need to create Task-based API's allowing developers consuming your library to take advantage of async if they want to (granted, if BC itself wants to take advantage of doing things asynchronously internally, that would be grounds for depending on .NET 4.5 imho)

So I guess the question comes down to: what does .NET 4.5 offer that we'd want to use inside BC?

.NET 2.0 offers a LOT of advantages over 1.1 such as the Dispose pattern for Streams, generics, etc. .NET 3.5 starts to add some LINQ which can be really handy .NET 4.0 adds some nice Stream API's such as CopyTo(Stream) (or was this added in .NET 3.5?), CancellationToken, System.Threading.Task, etc. .NET 4.5 adds async/await, but unless bouncy castle actually intends to use async internally, this is somewhat moot.

clairernovotny commented 8 years ago

.NET 4.5's most important characteristic is that it's a System.Runtime based target. That is, it can use contracts instead of mscorlib directly.

More to the point, you'll be able to use xProj to create multiple profiles in a single project:

Then you just have iOS/Android projects until they support netstandard1.3. Once Xamarin supports those contracts you won't need platform specific builds for them.

The doc on the .NET Standard Platform is here: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/standard-platform.md

jarz commented 8 years ago

@jstedfast async and await were added in .NET Framework 4.5. It'd be easier to use those than TPL -- I'm not sure TPL is maintained now.

ETA: I can't read -- you already mentioned that. D'oh!

clairernovotny commented 8 years ago

@peterdettman I have pushed a packaging update to the 1.8.1 version that includes support for netstandard.

https://github.com/onovotny/BouncyCastle-PCL/tree/pcl-netstandard

I plan on releasing this as 1.8.1.1 to the Portable.BouncyCastle package on Monday so that people can more easily use it with .NET Core.

clairernovotny commented 7 years ago

An update -- the netstandard version is here now: https://github.com/onovotny/bc-csharp/tree/netstandard

It's built on VS 2017's cross-compiling feature to simplify and reduce the number of projects needed.

Rajarch commented 7 years ago

Hi, just wanted to know if there are any plans by committers to extend the library to include SimonEngine.cs and SpeckEngine.cs on the lines of these cipher engines which are available in the java. Thanks

mtausig commented 6 years ago

@peterdettman Is there any timeline when this branching will be happening?

mdsitton commented 2 years ago

I would assume modernization patches can be merged now?

As of now the oldest supported version of .net framework from Microsoft is .net framework 4.6.2 which you can find this information here: https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-framework

Which checking here: https://docs.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0

This brings us up to .net standard 2,0 support as being the minimum supported version.

If i were managing the library in order to do a full modernization effort, I would dual target .net standard 2.0 and make use of System.Buffers for Memory/Span/ReadOnlySpan support: https://docs.microsoft.com/en-us/dotnet/api/system.buffers https://www.nuget.org/packages/System.Buffers/

The advantages of migrating internal code to use this is the massive performance and memory allocation reduction enhancements that can be achieved using these api's that can reduce thrashing of the heap and reduce garbage collection overhead on older .net framework versions (the coreclr gc is generally faster than older .net framework based runtimes)

I can help out with contributions in this area if this proposal would be considered.

mdsitton commented 2 years ago

Mentioning my suggestion of a full library modernization effort targeting a BC 2.0 release which would drop support for older .net releases. Would be a great time to also do a lot of modernization work to move to generic collections, or Span/Memory based apis potentially.

Since incrementing the major version number would be a good opportunity for doing an API compatibility break. (though i don't know what BC's policy is on such api deprecation/changes and whatnot.) Older versions can still be maintained in branches for critical bug-fixes and security fixes.

mdsitton commented 2 years ago

Additional changes that i think could be done in such a modernization effort is moving namespaces to just be BouncyCastle instead of Org.BouncyCastle as this type of Reverse DNS based naming is not utilized in the ,net world.

mdsitton commented 2 years ago

My suggestion for a modernization plan would be to start by splitting up the library into multiple smaller assemblies here's a small example of what Could be done:

BouncyCastle.Core - Any core utilities such as math, compression, collections etc BouncyCastle.Crypto - Low level crypto primitives BouncyCastle.Tls - Tls implementation BouncyCastle.OpenPgp - OpenPGP implementation BouncyCastle.Tsp - Time stamp protocol implementation BouncyCastle.Ocsp - OCSP implementation

This would be useful so that users can include only the implementations that are required for their code, reducing overall code size. Assemblies could also be tested independently as conversion work is being done. Assemblies would also depend on each other as required. the old crypto/tls implementation should be dropped in this process.

Then what can be done is starting at the lowest level primitives moving upwards being modernizing the actual implementations of higher level code making sure tests stay in place and functionality utilized from the core primitives is being converted to the new api's

mdsitton commented 2 years ago

I do think if a full modernization effort were to be done it would be worth also considering starting with .net standard 2.1 as the baseline, as there are additional major improvements that can be taken advantage of if this is used, such as nullable reference types and whatnot.

Though there are ways you can get some of these features with .net standard 2.0 as a lot of them are just compiler features. So as long as you are developing with newer runtimes then targeting .net standard 2.0 for older framework support: https://stu.dev/csharp8-doing-unsupported-things/

mdsitton commented 2 years ago

Another change that should probably be made in is moving any interfaces to be named as: IInterfaceName as currently they are just named like standard class and it is convention to utilize this naming scheme for .net code.

Finally i see the library is likely using nant for some kind of build/release automation, i would recommend migrating away from this tool as it seems to be no longer actively maintained and doesn't have specific support for the modern .net standard ecosystem. Moving tooling to using the modern dotnet CLI tool for any required building/build publishing needs: https://docs.microsoft.com/en-us/dotnet/core/tools/

Along those lines it would also be nice to have a CI pipeline setup that is public facing so that the test suite can be ran on each commit for better test visibility and validating pull requests, Something could at least be setup with github actions: https://github.com/marketplace/actions/setup-net-core-sdk

markusschaber commented 2 years ago

I do think if a full modernization effort were to be done it would be worth also considering starting with .net standard 2.1 as the baseline, as there are additional major improvements that can be taken advantage of if this is used, such as nullable reference types and whatnot.

.net standard 2.1 can only be seriously targeted if you use multi-targetting and conditional compilation, as the "classic" .NET Framework has been feature-frozen in version 4.8 and thus will never support .net standard 2.1, but still has a wide user base on windows with existing applications which cannot easily migrate away.

mdsitton commented 2 years ago

I do think if a full modernization effort were to be done it would be worth also considering starting with .net standard 2.1 as the baseline, as there are additional major improvements that can be taken advantage of if this is used, such as nullable reference types and whatnot.

.net standard 2.1 can only be seriously targeted if you use multi-targetting and conditional compilation, as the "classic" .NET Framework has been feature-frozen in version 4.8 and thus will never support .net standard 2.1, but still has a wide user base on windows with existing applications which cannot easily migrate away.

Yes which is why i mentioned some of this, with using newer compilers to use some newer features then also targeting down to .net standard 2.0

Xyncgas commented 2 years ago

Modernizing BouncyCatle api brings a huge benefit for the current decentralized computing trend, which often consume crypto libraries while running on edge devices :IoT / Mobile / Client In scenarios where concurrency is happening & UI needs to be responsive, it helps

mdsitton commented 1 year ago

Any update?

edit:

Looks like some work towards updating to newer .net frameworks is ongoing, as well as making use of Span. Which is great news!

peterdettman commented 1 year ago

A 2.0 release (as a NuGet package) targeting net60, netstandard2.0, net461 is imminent (as soon as our codesigning certificate arrives).

jstedfast commented 1 year ago

Should you target net462 instead of net461 since net461 was end-of-lifed back in April?

peterdettman commented 1 year ago

I was in two minds about it for just that reason (and switched back and forth), but I followed advice to provide net461 to cover users who had netstandard2.0 compatibility issues in the range [net461, net471]. net461 targeting packs are still downloadable for people to build from source, which is a consideration.

Is there any practical downside to targeting net461 (which doesn't force anyone to run on it)?

jstedfast commented 1 year ago

@peterdettman sounds reasonable to me. The only downside I know of is that older frameworks don't have the newer APIs which may or may not matter depending on whether the project authors want those features.

markusschaber commented 1 year ago

API-wise, there's not much difference between 4.6.1 an 4.6.2. If any API present in 4.6.2 or newer turns out to be of advantage, it can still be added as an additional multi-target.

peterdettman commented 1 year ago

Here it is: https://www.nuget.org/packages/BouncyCastle.Cryptography.

This release is dedicated to @clairernovotny for keeping the project alive these past few years with Portable.BouncyCastle. Although we didn't merge this PR, it was used as the point of departure and to help us figure out the new project formats and tooling. Thanks for all your work, Claire.

tranb3r commented 1 year ago

My suggestion for a modernization plan would be to start by splitting up the library into multiple smaller assemblies

Is this split going to be done ?

peterdettman commented 1 year ago

@tranb3r We are not currently considering any splitting in this version (2). It's something we definitely want to do for the next major version (v3 presumably), loosely aimed at Q1, '24, if not earlier.

Evengard commented 1 year ago

Are there any plans on making the Stream routines async? Blocking in 2023 is kinda... Weird.

peterdettman commented 1 year ago

@Evengard It would be helpful to list a few example methods you would (presumably) like async variants for?

Evengard commented 1 year ago

Mostly the ones accepting a TextReader or arbitrary file paths, aka the PemReader class can be asyncified. Also ones which writes to files (PemWriter as an example). Theese are the ones which I found out, it seems there isn't a lot of things in dire need of asyncification.