Move the password hashing logic

[blindmonkey]

I hope that this isn't unwelcome, but I noticed that when a user tried to login with a username that doesn't exist, the call immediately fails. This could potentially leak existence information to an attacker. In order to avoid this, I moved the guard let hash = match.hashedPassword to before the username check, so it always takes at least that long before the login fails.

[blindmonkey]

This might actually be a bit trickier than I thought. The BCryptHasher expects the hashes to be in a consistent format, and if other hashers that can be used also expect a specific format, then hardcoding a bogus password hash to compare isn't going to work. I'm open to suggestions, but it seems like the right approach here would be to expose a passwordHasher field in addition to a passwordVerifier?

[tanner0101]

With password hashing, wouldn't we again run into a potential leak? If the user exists, password is verified if the user doesn't, a password is hashed and verified.

Maybe we can add an additional field to the password verifier, like bogusPassword? That way each verifier is responsible for supplying a valid hash.

[blindmonkey]

@tanner0101 You're right. I'm not quite as knowledgable about how things should be done in this project as you are, but putting a bogus password on the verifier somehow doesn't feel right to me. What do you think about adding a var bogusPasswordHash: String? to PasswordAuthenticatable, and simply defaulting to the old behavior if it doesn't exist? That way it could provide an easy way to not leak information.

Though I'm a bit torn at this point, since it would require the developer to know that that's the state of affairs and wouldn't provide the best behavior as a default.

[tanner0101]

Would be a pretty useless fix if it also required developer intervention to go into effect. I think a protocol requirement on PasswordVerifier is the best we can do here.

Some options:

• bogusPasswordHash
• simulatePasswordVerifiation()

[0xTim]

So my preference for this is to not make it the responsibility of the framework. Most network requests should probably hide most of the delay and if an app is especially concerned about leaking this information they can implement their own authenticate logic.

It's worth noting that some providers (e.g. Google) are moving towards validating the email first

[tanner0101]

@blindmonkey thanks for the contribution!

Closing this for a couple of reasons:

1: Implementation as posted still leaks information. 2: This seems like something that potentially falls outside of the responsibility of the framework. 3: Lack of activity / unresolved merge conflicts.

Any information/links regarding other web frameworks doing this that could set a precedent for Vapor to follow would be appreciated. Otherwise, I'd be willing to take another look at a new implementation that does not continue to leak information.