search

padrino / padrino-framework

Padrino is a full-stack ruby framework built upon Sinatra.
http://www.padrinorb.com
MIT License
3.36k stars 510 forks source link

Account should not store decrypt-able passwords. #121

Closed nesquena closed 13 years ago

nesquena commented 14 years ago

Brad Greenlee raised an interesting point on twitter:

http://twitter.com/bgreenlee/statuses/12593859916

@padrinorb was looking pretty nice until I saw this http://foo.tl/cn93BM and this http://foo.tl/cSFuW8 . Ugh. @padrinorb There is no reason to ever have your password be decryptable or stored in plaintext (which is essentially what you're doing). @padrinorb See http://foo.tl/9lG9EN and http://foo.tl/bN4iIt and a hundred similar articles.

Essentially he raises an issue with storing passwords that can be decrypted. Personally I have always stored passwords in a hashed format with a salt that can never be recovered. The following gives some rationale of why we did this:

This util it's used for encrypt/decrypt password. We want password decryptable because generally for our sites we have: password_lost. We prefer send original password instead reset them.

Personally to me this seems like a tradeoff not worth making. What do you guys think? Shall we make the 'sensible' default here to use a bcrypt undecipherable hashing algorithm + a reset?

bgreenlee commented 14 years ago

Thanks for listening. While you're at it, you should probably set a good example and use bcrypt instead of SHAx. See http://codahale.com/how-to-safely-store-a-password/ and http://bcrypt-ruby.rubyforge.org/

nesquena commented 14 years ago

Yes, excellent suggestion regarding bcrypt over SHA. I have done this before in the past but it slipped my mind when creating this issue. Thanks again for raising this issue with us.

DAddYE commented 14 years ago

First Thanks Brad for your feedback.

I totally agree with you and Nathan.

We can make as default an undecipherable hashing algorithm.

Is important remember that Padrino itself don't force you to use our own "way" encrypt/decrypt is stored in the account model but if we are pefectionist we can extend/override actual basic crypt

So In my opinion we need to use one undecipherable algorithm and then leave the user free to change them and write their own... if necessary.

Can be done guys?

nesquena commented 14 years ago

I noticed that this (in my old plugin) http://github.com/nesquena/sinatra_more/tree/master/generators/components/orms/ has a basic idea of how to do crypted_passwords.

Here is a basic gist of how it might work in AR: http://gist.github.com/374489 . Thoughts?

DAddYE commented 14 years ago

Nathan, it's perfect! Brad?

bgreenlee commented 14 years ago

Looks good to me. Thanks, guys!

nesquena commented 14 years ago

Great then! It seems to me the only thing we need to do then is add a requirement on bcrypt-ruby to Gemfile? and then change admin to use this pattern / attributes (crypted_password) for each orm. DAddYE this seem ok to you?

I agree that this should be pluggable for cases where the user forces us to keep decipherable passwords as you explained if they demand a way to retrieve their password. But this way should definitely become the default and people can hopefully easily override if necessary.

DAddYE commented 14 years ago

Great then! It seems to me the only thing we need to do then is add a requirement on bcrypt-ruby to Gemfile? and then change admin to use this pattern / attributes (crypted_password) for each orm. DAddYE this seem ok to you?

Yep! Seem perfect for me.

Tomorrow (if you not already do that) I will apply this.

This will be included in 0.9.10 or 0.9.11 ?

nesquena commented 14 years ago

Probably best to include it in 0.9.11 I think? 0.9.10 is so close to release and already tested. I will let you do it tomorrow. Thanks!

DAddYE commented 14 years ago

Consider also that this "modification" stop backward compatibility.

nesquena commented 14 years ago

Yes, I see your point but better to make the change now while things are still early on. This will be a much better default for the future. Can you think of an easy way to prevent this from breaking older apps?

DAddYE commented 14 years ago

Yep, I think we only need to leave this: http://github.com/padrino/padrino-framework/blob/master/padrino-admin/lib/padrino-admin/utils/crypt.rb that can be useful for some simple internal crypt/decrypt

nesquena commented 14 years ago

Agreed on leaving the crypt utility in there! Thanks.

bgreenlee commented 14 years ago

You could provide a db migration task to allow people to convert their encrypted passwords to the bcrypt version.

DAddYE commented 14 years ago

Yep this can be also useful. Thanks!

aaronlerch commented 13 years ago

I posted a comment about this to the google group (for some reason I hadn't found this already-existing issue). But I generated an app a day or so ago and I see things still use the old (bad) behavior. What's the status of this issue? I'm happy to fix it if it's been stalled... Thanks!

DAddYE commented 13 years ago

bcrypt now is not available for ruby 1.9.2 and I think also for jruby.

So what do u suggest?

bgreenlee commented 13 years ago

I just tested bcrypt-ruby under ruby 1.9.2 and jruby 1.5.1 and it worked fine. What issues are you seeing?

sob commented 13 years ago

I've got a branch working on 1.9.2 sob/padrino-framework@a68ebc487cfe28bcbcbd - do you want me to send a pull request?

DAddYE commented 13 years ago

Yep! Finally can we close (after my revision) this odd thicket. Thanks sob and thanks community!