On 09/02/2011 01:53 PM, Tomas Sedovic wrote:
On 08/31/2011 01:54 PM, jprovazn(a)redhat.com wrote:
> 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
Looking good, ACK.
Hi, thanks for the review,
First, the Pool Family permissions patch was pushed after you sent
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
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:
If Warden doesn't provide us with default secure way to encrypt
password, we should just go ahead and use bcrypt:
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).