heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
23.83k stars 5.54k forks source link

Argon2 encryptor #5579

Open RobertDober opened 1 year ago

RobertDober commented 1 year ago

Hello there

This would be better as a PR, but as it is based on a tag (which we need) that does not work, so please forgive me to discuss it here:

https://github.com/heartcombo/devise/compare/v4.8.1...RobertDober:argon2-4.8.1-draft?expand=1

This is (yet) more a question then an implementation, although preliminary tests with our application are promising.

AAMOF my organization is pushed by one of our clients to not use bcrypt but argon2 for password hashing. Although we are perfectly happy with bcrypt we have a fork running with Argon2 replacing Bcrypt.

But we have no intention to maintain a fork of devise, so we thought, that maybe you would be interested in integrating argon2 and I have implemented it as can be seen above.

Of course the argon2 dependency could be removed from the PR by making the encryptor API public which is just the two module methods digest and compare. OTOH I wanted to share the Argon code too if you were interested.

I would appreciate any feedback in oreder to avoid any unnecessary waste of time, if however you are interested our organization would be most happy to give back to devise which is my second preferred José Valim project, (sorry Ruby) you can guess which one is the first ;).

The PR would then be against main or any preferred branch, we would love to have it in an update of 4.8.x too of course because not yet Rails 7 :sob:

Thank you in advance and for devise

Robert

carlosantoniodasilva commented 1 year ago

@RobertDober sorry about the delay, I'm replaying here on the issue to keep the discussion going.

Appreciate your work on implementing an Argon2 compatible encrypting with Devise. I myself am not much familiar with Argon2 yet, have to do some catching up there.

The way we generally setup different encryptors with Devise is via a separate devise-encryptable library. Up until now all the encryption libs have been older encryptors like SHA & other compatibility things, so it might need some tweaks for better compatibility with something like Argon2 (for example it sort of requires a separate "password salt" since all of them currently do, but bcrypt doesn't, and as far as I can tell argon doesn't either.)

In any case, ideally any new encryption mechanism that's not the default bcrypt should live there. If we implement something in Devise again it will conflict with that infrastructure so it'd have to be taken into account, which is why it might be preferable to keep those there for now.

RobertDober commented 1 year ago

@carlosantoniodasilva thank you for your reply. I had been looking into devise-encryptable but did not understand it at first. I will be crafting out a PR today.

Até logo

RobertDober commented 1 year ago

Oh and I forgot to mention, I also did not look into it too much because its last release in 2014, would you consider to release a new version with Argon2?

carlosantoniodasilva commented 1 year ago

would you consider to release a new version with Argon2?

Yup, that would not be a problem. (admittedly there are some PRs there I need to look into, that might warrant new releases at some point, I need to circle back on them eventually.)

RobertDober commented 1 year ago

with all due respect and by no means demanding anything (you might know me as an oos developper and therefore understanding whatever reason you might have not to accept my PR #5581, at least the burden of maintaining more code) I would like to make the following observation, and sorry if it is wrong

Would the PR not allow a more general way to change encryptors, totally dissasociating the encryptor from the ORM.

But again I am of course pushing the agenda for my company here and will not bother you anymore if the value of the PR is just not enough, believe me I understand, nobody will help you maintaining the code and noboday shall put more pressure than what I already put.

E obrigado para todo isso que ya facêm (that is probably all wrong but anyway)

And of course I can also understand when your reply will be delayed.