crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.47k stars 1.62k forks source link

Add Argon2 password hashing implementation to Standard Library #7676

Closed elaine-jackson closed 5 years ago

elaine-jackson commented 5 years ago

The standard library for Crystal includes a few cryptographic functions including password hashing using the Bcrypt algorithm (See: https://github.com/crystal-lang/crystal/blob/60760a5460aa13a1f80c5a1a7410782dc4076b77/src/crypto/bcrypt.cr) this is very useful. The algorithm Argon2 (https://github.com/P-H-C/phc-winner-argon2) won the password hashing competition which was intended to find a successor to Bcrypt. Not that there is anything wrong with using Bcrypt but Argon2 builds upon what we learned with Bcrypt and makes password hashing even better for everyone. What do we think about adding Argon2 into the Crypto part of the Crystal Standard Library?

asterite commented 5 years ago

I think it should be in a shard. The code won't be written by us so we won't have a way to maintain it in case the original maintainer disappears or has no time.

j8r commented 5 years ago

Having arbitrary algorithms in the stdlib because some particular core members have implemented them and take the responsibility to maintain them (forever?) is a bit flawed. What happens (s)he has no time to maintain it, and/or if there are no longer core members that have enough knowledge of this very algorithms?

I think this shouldn't be the responsibility of the core team to maintain Crypto, at all. This can have serious security implications and require a deep, specific understanding of how this algorithms works. And they are quite simple compared to OpenSSL, which appears to be maintenance-heavy, as @RX14 point out

Rust has chosen to not support crypto. On the other hand, Go has https://github.com/golang/crypto, but has the Google company behind. Ruby uses only OpenSSL.

There is already https://github.com/ysbaddaden/scrypt-crystal, why won't each crypto algorithm owned by its implementer?

ysbaddaden commented 5 years ago

Before even considering inclusion into stdlib there should at least be a working shard.

Also, bcrypt may be moved out, so it's kinda going circles to discuss to include an alternative.

Note: this has nothing to do with Argon2 which is an excellent password hashing toolset.

j8r commented 5 years ago

Another alternative is to work/wait for https://github.com/openssl/openssl/issues/4091

elaine-jackson commented 5 years ago

I think it should be in a shard. The code won't be written by us so we won't have a way to maintain it in case the original maintainer disappears or has no time.

Understandable but you could say the same about any feature in the standard library. Should we stop maintaining HTTP::Client because one day the maintainer might quit?

Before even considering inclusion into stdlib there should at least be a working shard.

Also, bcrypt may be moved out, so it's kinda going circles to discuss to include an alternative.

Note: this has nothing to do with Argon2 which is an excellent password hashing toolset.

This worries me. Amber (https://amberframework.org) currently uses Bcrypt from the standard library for authentication. Please don't remove features that are in widespread use from the standard library.

Another alternative is to work/wait for openssl/openssl#4091

Honestly that may be a better idea that'll save the Crystal team time and effort otherwise needed to maintain our own implementation.

Having arbitrary algorithms in the stdlib because some particular core members have implemented them and take the responsibility to maintain them (forever?) is a bit flawed. What happens (s)he has no time to maintain it, and/or if there are no longer core members that have enough knowledge of this very algorithms?

That's a problem I'm not sure about the solution. I go back to should we remove HTTP::Client once the maintainers for it are gone.

I think this shouldn't be the responsibility of the core team to maintain Crypto, at all. This can have serious security implications and require a deep, specific understanding of how this algorithms works.

I agree the security implications are severe. Once you add something to the standard library it's problematic to remove as so many things will rely on it.

And they are quite simple compared to OpenSSL, which appears to be maintenance-heavy, as @RX14 point out

Rust has chosen to not support crypto.

Maybe like them we should offload to OpenSSL

On the other hand, Go has https://github.com/golang/crypto, but has the Google company behind. Like you said Go has a huge corporate developer team backing it. Crystal does not.

Ruby uses only OpenSSL.

We're Ruby like, qw might as well copy the using OpenSSL part too 😛

There is already https://github.com/ysbaddaden/scrypt-crystal, why won't each crypto algorithm owned by its implementer?

Goodpoint. But including it in standard library would make it easier than having to find the name of and add the shard. Individuals could still maintain as part of the standard library. Is this the right approach?

j8r commented 5 years ago

As @ysbaddaden said, this issue can be considered stalled, waiting a Crystal implementation or an OpenSSL API before considering to include it to the stdlib - there is nothing yet! For the time being, those interested can also create bindings for the official reference implementation.

Sija commented 5 years ago

Another alternative is to work/wait for openssl/openssl#4091

Honestly that may be a better idea that'll save the Crystal team time and effort otherwise needed to maintain our own implementation.

Given how's ecosystem fragmented, I'd say that waiting for openssl support is a road paved with frustration. Even if they'd release it tmrw (and they're not), it would become usable on major OS distributions after months, or years (what's visible for example in case of building nginx with TLSv1.3 support, which requires openssl 1.1.1, on currently newest, debian 9).

IMO building a shard, which might get sucked into the stdlib after some grace period sounds as the most sensible and realistic scenario to move things forward.

konovod commented 5 years ago

If someone desperately needs Argon2i, monocypher has it's implementation (with zero dependencies) and there are bindings for it.

asterite commented 5 years ago

Understandable but you could say the same about any feature in the standard library. Should we stop maintaining HTTP::Client because one day the maintainer might quit?

Yes, I'm now in the opinion that things that are not core to the language should go into shards and be maintained by core team members plus community, or just community. You can see that HTTP::Client and HTTP::Server are already lacking some features and things don't move forward, and this is mainly because the core team member doesn't have the time and resources to tackle all of it. Move most things to shards and the community as a whole can evolve them.

ysbaddaden commented 5 years ago

I'm closing. Please implement Argon2 and Blake2 as pure Crystal, or create a libsodium bindings (forget libssl). In any case, this should be a shard.