bcgit / bc-csharp

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

Argon2 #559

Closed zer0x64 closed 1 month ago

zer0x64 commented 2 months ago

Describe your changes

Added support for the Argon2 password-based key derivation function.

How has this been tested?

Right now, I implemented tests for the test cases in the RFC. I can add more test cases if needed.

Checklist before requesting a review

zer0x64 commented 2 months ago

Related issue: https://github.com/bcgit/bc-csharp/issues/202

zer0x64 commented 2 months ago

Putting this as a draft right now. I got the RFC test cases working, but I still have to cleanup my code, optimize it and add multithreading support. I just wanted to open as a draft ASAP to be able to address requested changes.

One thing that must be brought to attention is multithreading support. I'd assume we want to make multithreading support optionnal, for portability, but I was wondering how you'd like that to be implemented? Via an additionnal parameter on the API, or via a preprocessor directive?

On thing I'd also like to fix before merging is using Span and MemoryMarshal to reference byte[]<->ulong[] arrays directly instead of copying when on a little endian machine. Another thing is that I don't support memory clearing yet, but that should be easy to add.

zer0x64 commented 2 months ago

Ready for review! A couple of notes:

  1. Right now, multithreading is enabled if there is more than one lane. Do we want to put an (opt-out) argument or preprocessor directive to disable multithreading support?
  2. Technically, since Blake2b works on QWORDs too, it might be possible to optimize for big endian and framework platforms by implementing and using a QWORDs based api for when Blake2b is used. However, this is not used for the intensive part of the algorithm so I don't think it's too important.
  3. Do we want to add a user-facing Span<byte> API for netcore3.1+ users? That could be work for a future PR. Note that this could be very complicated because Span don't play well with class and we have byte[] in the Argon2Parameters class.
  4. My Zeroize<T> generic function might be a better fit in some utils class, like an Arrays.Clear() overload.
peterdettman commented 1 month ago

I just merged an earlier Argon2 PR: https://github.com/bcgit/bc-csharp/pull/520 . I see there are a few diffs in your PR (e.g. span-related and tasks) and will probably incorporate those.

zer0x64 commented 1 month ago

Great news! I don't mind opening up a new PR to add those improvements myself, just tell me if you'd rather have me do it or do it yourself!

peterdettman commented 1 month ago

@zer0x64 A PR adding improvements would be welcome.

zer0x64 commented 1 month ago

This PR got automatically closed because I've put the work on a branch on my fork instead of master, but the code is still over there. I'll open a new PR to add the requested changes