Password checking maybe broken about act

[waterkip]

No, I'm working on this. I want to use Argon2 and use some OWASP recommendations to store passwords. I reverted the change earlier this week to use bcrypt again,

[HaraldJoerg]

As far as I found in the records, the change from MD5 to BCRYPT was done in 2011 (853dfbe). The current handling of the salt in this branch is broken. Are you still working on this or should I put it on my todo list?

[vanHoesel]

I think it would be great to just only have changes related to get an easy to install and runnable PSGI based version.

Any other improvements and and less important issues that are applicable for legacy and psgi would be needed to be postponed and kept in sync

[HaraldJoerg]

Fixing this can be considered as part of the "easy to install and runnable PSGI based version". Right now there are two creepy (=undocumented, untested, and insecure) parameters which must be set in the global act.ini.

The algorithm doesn't bother me too much, in my opinion Bcrypt is good enough for the risk profile of a conference site. It's the salt handling which is broken. As an easy intermediate I'll create a fix which derives the salt from the login name. The change is easy, local and doesn't need an extra column in the users table for a secure random salt (with all the accompanying changes for DB init and migration).

Legacy, by the way, seems to use MD5 without a salt. So, no bonus for security, but also no penalty for bogus configuration values.

[waterkip]

I think I fixed it, could you check @HaraldJoerg ?

I didn't use Argon2 yet, as the implementation cannot be used with a default Authen::Passphrase instance, so I stayed at bcrypt.

[HaraldJoerg]

Looks good!

One remarks, though:

You are still referencing the bcrypt cost parameter:
cost => $Config->bcrypt_cost // 8,
This means that if you don't have it defined in your act.ini, then every request will throw out a warning - that's the way AppConfig and Act::Config work. I suggest to just drop that parameter and stick with a constant value of 8. I doubt that whoever runs an Act site will want to delve sufficiently deep into the crypto stuff that she can make an informed decision about value... and it's easy to mess up things if you change the value in a conference act.ini.

[waterkip]

Cool! Fixed that.

I'm closing this one due to your "All OK" message :)