Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Argon2id binding #24

Closed
midnight-wonderer opened this issue Aug 31, 2018 · 17 comments
Closed

Argon2id binding #24

midnight-wonderer opened this issue Aug 31, 2018 · 17 comments

Comments

@midnight-wonderer
Copy link

Quote from the IETF draft.
https://tools.ietf.org/html/draft-irtf-cfrg-argon2-03#section-9.4

Recommendations
The Argon2id variant with t=1 and maximum available memory is
recommended as a default setting for all environments. This setting
is secure against side-channel attacks and maximizes adversarial
costs on dedicated brute-force hardware.

I wonder if we can get the variant to be the default of ruby-argon2 too?

@technion
Copy link
Owner

Definitely a good idea.
It's probably a better default based on the above - which is a change from the earlier recommendation.

Give me a bit of time to work through this.

@technion
Copy link
Owner

Plot twist: The reference C library has no tests for Argon 2id. Let me get a PR for that first.

@technion
Copy link
Owner

@MidnightWonderer Just updating to note I've sent P-H-C/phc-winner-argon2#261 in to address the upstream issue, waiting on merge or feedback.

@technion
Copy link
Owner

@midnight-wonderer Thanks for your patience with this. As you may have seen, my PR has been taken upstream.

I've pushed some commits to integrate support for 2id in our C bindings, and I'll be integrating verify support soon. Changing the actual default will take a major version bump, but we'll get there.

@ankane
Copy link
Contributor

ankane commented Dec 4, 2018

Thanks for all the updates @technion 👍 I'm excited to see this as well (and may make it the default for blind index in the future).

@midnight-wonderer
Copy link
Author

@ankane are you planning to upgrade from argon2i to argon2id for your indexes?
Does this actually a bad thing for indexing use cases?

@ankane
Copy link
Contributor

ankane commented Dec 4, 2018

@midnight-wonderer Planning to add a new algorithm type for it (to mirror CipherSweet blind indexes). What are you thinking?

@technion
Copy link
Owner

technion commented Dec 4, 2018

I have tagged v1.2.0 which now supports verifying Argon2id hashes.

I will shift the default in master shortly but I'll take my time with tagging 2.0 and ensure it's correct.

@ankane
Copy link
Contributor

ankane commented Dec 4, 2018

Thanks @technion 💯 Are there plans to add support for creating Argon2id hashes (hash_argon2id)?

@midnight-wonderer
Copy link
Author

@ankane I think that changing the algorithm would have the same result as changing salt (or key).
Not many people would decide to do it in their production environment.

This will be a bit off topic but just my 2 cents:
The indexing is a misuse of Argon2 in the first place IMHO. (There are parameters apart from salt stored in the encrypted data. e.g., algorithm version)
Encrypting just the password with Argon2 with a proper randomized salt, and never index any password, is what I would recommend.

@ankane
Copy link
Contributor

ankane commented Dec 4, 2018

I appreciate the feedback.

  1. As you mention, current users would need to rotate the blind index to use the new algorithm, but I think it's a good choice for new users.

  2. Two common use cases for Argon2 are password storage and key derivation. Blind indexing uses the second. https://libsodium.gitbook.io/doc/password_hashing/the_argon2i_function

@technion
Copy link
Owner

technion commented Dec 4, 2018

@ankane absolutely, I'll be making Argon2id the default in 2.0.

@technion
Copy link
Owner

technion commented Jan 2, 2019

Update: master now hashes to Argon2_id, whilst maintaining full backward compat. This feature needs better tests before its'fully baked.

@ankane
Copy link
Contributor

ankane commented Jan 2, 2019

@technion Awesome! I think it'd be good to have a separate function like hash_argon2id_encode so Argon2i can still be used when desired.

@technion
Copy link
Owner

technion commented Jan 2, 2019

@ankane I've had a goal here of not giving someone enough rope to hang themselves.

I certainly appreciate the need to support verifying all formats, but I can refer to the amount of in production code I've been told apparently uses the _salt_do_not_supply parameter in describing why just following current best practice is a better proposal.

Edit: To be clear though, this is a change that will necessitate a v2.0 version, and I'll keep 1.x supported as long as feasible. So if this matters, there is that option.

@ankane
Copy link
Contributor

ankane commented Jan 2, 2019

I completely agree that cryptography libraries should be designed to be hard to misuse, but don't think this falls into that category.

Regardless, thanks for adding Argon2id support 🎉

@technion
Copy link
Owner

technion commented Jan 3, 2019

I'll close this off as master has covered everything I believe is necessary. I'll tag 2.0 shortly for release.

@technion technion closed this as completed Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants