Page MenuHomePhabricator

AuthManager cannot audit passwords
Closed, DeclinedPublic

Description

Before AuthManager, the LoginAuthenticateAudit hook provided the password; that can be useful to recognize attacks (e.g. by matching them against lists used by cracking tools) or to get statistics for password policy changes. AuthManager replaced that with AuthManagerLoginAuthenticateAudit which does not include the requests, only the response.

Event Timeline

Given we now have implemented the complexity requirements, I don't think its particularly important. The important thing is we can audit failed attempts in order to detect cracking attempts.

As currently implemented, we don't have access to the password on a failed attempt, but we do have the error message, so we know it failed because of an invalid password. Is that OK?

@csteipp was the one who used to monitor that log (afaik) so im not sure how useful logging the passwords was. But my personal view is recording passwords, even wrong ones, is sketch, so i think the change is fine.

As currently implemented, we don't have access to the password on a failed attempt, but we do have the error message, so we know it failed because of an invalid password. Is that OK?

Yes, I believe that's enough. I don't see a need to implement the ability to log passwords at this time.

I think the fewer places we have the password the better. So I think this is fine.

If we want to find out how many users are going to be affected by a change to a password policy again, we can make a special purpose pre-auth provider to log it.