On 09/02/2011 03:40 PM, Jan Provazník wrote:
On 09/02/2011 01:53 PM, Tomas Sedovic wrote:
On 08/31/2011 01:54 PM, jprovazn@redhat.com wrote:
https://www.aeolusproject.org/redmine/issues/2118 This patchset replaces authlogic with warden which is used also in katello. This is first step which allows us to add more auth strategies (ldap, oauth) or use devise on top of warden, this also synchronizes us with katello auth.
What's not very good in this solution is lack of password handling/validations, which authlogic handled so now we have to handle this ourselves. This can be solved in some next step by using devise which is based on warden.
Password reset (or db recreation) is required with this patch because encrypted password has different format.
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
Looking good, ACK.
3 notes:
Hi, thanks for the review,
First, the Pool Family permissions patch was pushed after you sent this and it doesn't have the tests fixed for the Warden switch, so some of the tests are failing (both rspec and cucumber).
Could you please make sure all tests pass before pushing this?
Second, the migrations here invalidate all the existing users -- they will not be able to log in again.
I'm not sure if we need to worry about that now, especially since the fix probably won't be very easy. But it's good to keep it in mind.
I don't think it is worth trying to do some conversion. Moving notice into commit log should be sufficient (currently this notice in in first mail).
Lastly, we shouldn't be implementing our own password crypto (the util/password.rb file). It looks okay on glance (at least we're stretching the hashes), but it's a slippery slope.
Actually it's not our own lib - it's taken from katello (as other chunks of code), which took it from third party (maybe: http://www.zacharyfox.com/blog/ruby-on-rails/password-hashing).
Yeah, I realize that you didn't write this. The "we" was meant as Aeolus + Katello.
If Warden doesn't provide us with default secure way to encrypt password, we should just go ahead and use bcrypt:
https://github.com/codahale/bcrypt-ruby
It's packaged in Fedora, the technology has been vetted by security researchers over the years and we will not be bitten in the arse if we go with it. Can't say the same about a homebrew solution that had right about zero cryptographers banging at it.
We don't have to implement bcrypt in this patchset -- go ahead and push this. But we should do it soon. Definitely before any okay-for-public-consumption releases. Note that Devise uses brcypt by default so if we switch to that, this problem is solved.
I created a Redmine task so we can track it:
Well, sure - we can consider using bcrypt, though this solves only password encrytion problem and diverges us from katello. I would prefer to use devise in conductor and katello, which can handle password encryption too (among other things).
Devise does all the right things by default. But if we do not go down that road, we (and Katello) should really switch to to brcypt instead of carrying and maintaining potentially insecure code.
Thomas
Jan _______________________________________________ aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel