erdostom / devise-argon2 Goto Github PK

I'm looking to upgrade my app from the straight devise bcrypt to Argon2, and saw this gem, perfect!..

However after adding it and trying to get testing working I found this:

https://www.reddit.com/r/ruby/comments/415u5u/argon2_won_the_password_hashing_competition_heres/

which strongly implies that this gem does not implement Argon2 correctly. I don't know enough about this stuff to be able to say one way or another who is right there(and I certainly don't mean to insult the creators/maintainers of this gem), but I don't want to be using something which could put users security at risk. Am I safe to use this gem?

In version 1.0.0 of the ruby-argon2 gem the API to create a password changed from #hash to #create see https://github.com/technion/ruby-argon2/blob/master/lib%2Fargon2.rb .

This change breaks this gem.

Do you have a solution for building the gem on a windows based platform?

Installing argon2 0.1.4 with native extensions

Gem::Ext::BuildError: ERROR: Failed to build gem native extension.

C:/Ruby217/Ruby21/bin/ruby.exe extconf.rb

make "DESTDIR=" clean
rm -f tests libargon2_wrap.dll

make "DESTDIR="
gcc -std=c89 -pthread -O3 -Wall -g -march=native -shared -Wl,--out-implib,libargon2_wrap.dll.a ../phc-winner-argon2/src/argon2.c ../phc-winner-argon2/src/core.c ../phc-winner-argon2/src/blake2/blake2b.c ../phc-winner-argon2/src/thread.c ../phc-winner-argon2/src/encoding.c argon_wrap.c ../phc-winner-argon2/src/opt.c -o libargon2_wrap.dll
c:/ruby217/devkit3/mingw/bin/../lib/gcc/i686-w64-mingw32/4.7.2/../../../../i686-w64-mingw32/lib/../lib/libpthread.dll.a: could not read symbols: Archive has no index; run ranlib to add one
collect2.exe: error: ld returned 1 exit status
make: *** [libs] Error 1

make failed, exit code 2

An error occurred while installing argon2 (0.1.4), and Bundler cannot continue.
Make sure that gem install argon2 -v '0.1.4' succeeds before bundling.

Hi @erdostom

If Devise.paranoid is set to true, Devise makes sure that a given password is hashed whether or not a record with the given email is found. Hence response times are always the same and users can't infer if the account exists without having the password. (https://github.com/heartcombo/devise/blob/e2242a95f3bb2e68ec0e9a064238ff7af6429545/lib/devise/strategies/database_authenticatable.rb#L22)

I'm afraid we break this feature when we update work factors.

• When increasing work factors: Password verification for existing records (which have password hashes with the old work factors) takes less time than the hashing due to Devise's paranoid feature.
• When decreasing work factors: Password verification for existing records takes more time than the hashing due to Devise's paranoid feature.

I'd propose the following solution: We offer a new argon_options key, say :min_hashing_time, and we make sure that the hashing in password_digest and verify_password runs at least as long as the value of this option.

I suppose that the usual use case is increasing work factors. In this case, min_hashing_time should be set to the execution time of the hashing procedure with the new work factors. For this we can write a class method like set_min_hashing_time. Then the feature could be used like this:

class User < ApplicationRecord
  devise :database_authenticatable,
    :argon2,
    argon2_options: { [updated work factors] }

  set_min_hashing_time
end

(On second thought: It is probably not a good idea to set min_hashing_time like this. If the server is under heavy load, we might over-estimate the required hashing time, leading to performance problems and DoS opportunities. It might be better to always hard-code the value of min_hashing_time.)

I have implemented this solution (4212d43) and also wrote some tests to make the problem clear (e94cf79). However, the tests are flaky since execution time varies so I think we should not use them.

If you like the solution, I'll make a proper pull request and also write something about it in the README.

Right now, parallelism is hardcoded to 1. It would be great if this could be specified along with t_cost and m_cost.

Hi there,

as far as I understand it, devise-encryptable deals mainly with managing salts for legacy algorithms. Argon2 on the other hand has a built-in salt that is saved in the encrypted_password field (see https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md). So I think it might be a good idea to get rid of the devise-encryptable dependency altogether.

I played a little with a fork of this repository and it seems to be as simple as this: moritzhoeppner@a8b4542

Would you be interested in general in doing something like this? I'd be happy to make a pull request but I wanted to check before investing more time in this idea.

At this occasion we could make a few more improvements:

• Don't simply concatenate password and pepper but give the pepper as 'secret' parameter to Argon2::Password.new.
• Make the Argon2 parameters accessible via devise config.
• Allow migrating from bcrypt/different parameter values for argon2 by updating encrypted_password in valid_password? if the given password is valid.

Hi @erdostom,
the repository description is still "A devise-encryptable password encryptor that uses Argon2", which is also the link text on Google. Could you update the description? Maybe something like "Hash Devise passwords with Argon2"?
Thank you! :)

Hashing is not encryption.

In general, you don't encrypt passwords, you use a password hashing algorithm. The current terminology is incredibly likely to muddy the waters for your users.

Not that it is a huge issue, but the argon2 gem is already salting the password. If you do a search on the argon gem for salt_do_not_supply, you can see the line where you can ignore this step. Currently with this gem we're double salting. Might make sense to either defer to this gem for salting and then we don't need the extra migration that devise_encryptor gem wants? Is it possible to not use that migration?