Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yesod.Auth.Util.PasswordStore update? #1675

Open
Vlix opened this issue May 24, 2020 · 8 comments
Open

Yesod.Auth.Util.PasswordStore update? #1675

Vlix opened this issue May 24, 2020 · 8 comments

Comments

@Vlix
Copy link
Contributor

Vlix commented May 24, 2020

We've been working hard on the password-2.0 package and it might be a good idea to revisit the Yesod.Auth.Util.PasswordStore module, since it's not been touched for 3 years (and hasn't actually changed since 2014, when it was copied wholesale from pwstore-fast) and uses PBKDF, which, as far as I can gather from "the internet", is not the best way to store passwords nowadays.

There could be legitimate reasons to keep the module as it is, of course, but I just wanted to give a heads up.
(password-2.0 also uses cryptonite so it wouldn't induce a significant dependency burden, IMHO)

@snoyberg
Copy link
Member

If someone is interested in taking a crack at moving this over to password, I'm on board. Awesome to see some quality work on this, thank you for providing that package!

@Vlix
Copy link
Contributor Author

Vlix commented May 24, 2020

Thank you for the kind words.
For anyone that'll try to pick this up, what's the goal of the PasswordStore module?

The only thing this module seems to do that password-2.0 does not, is a strengthenPassword function. (though that functionality is on the roadmap for the password package)
Would a big PSA at the top of the documentation help that just directs users of this module to use the password package instead?

@snoyberg
Copy link
Member

It's simply meant to provide the functionality of pwstore-fast that we needed in yesod-auth

@Vlix
Copy link
Contributor Author

Vlix commented May 25, 2020

Aaah, I see, I didn't notice it was used in other modules of Yesod-Auth. (Only Yesod.Auth.Email AFAICT)

Ok, so if the PasswordStore module is either removed, or deprecated, in favor of password-2.0, it will probably need a major version bump, since even if updating the code can be done without changing the API, changing the algorithm will break compatibility with passwords already made using the current yesod-auth package. Though I'll expect the relevant type signatures to also change
(e.g. SaltedPass -> PasswordHash algorithm)

Come to think of it. The YesodAuthEmail class will probably need another type family definition type AuthAlgorithm site to define which algorithm the user wants to use, so that it can be used in type signatures: PasswordHash (AuthAlgorithm site)

This probably will also make it impossible to have a default definition of hashAndSaltPassword... 🤔

@paul-rouse
Copy link
Contributor

In yesod-auth-hashdb, I would want to maintain verification compatibility with previously generated passwords for a significant time, perhaps by adding some sort of conversion layer.

Is there any combination of parameters which makes the password-2.0 PBKDF2 give the same underlying hash as the implementation in Yesod.Auth.Util.PasswordStore? Obviously it's SHA256, but I can't find values which work for the other parameters at the moment. I would have thought there ought to be a way of getting the same answer from both implementations, if they really do adhere to the PBKDF2 spec, but maybe I'm missing something. (I haven't yet had time to dig into the code in detail.)

@Vlix
Copy link
Contributor Author

Vlix commented May 27, 2020

Supporting different hashes is also on the roadmap, but at the moment there's no way to do that.

You can see in this list that tons of different ways are used of constructing a hash with its parameters.
What you store in the DB is almost never included in a spec. The "password" algorithm specs almost always describe how to hash a string of bytes with certain parameters, which results in another string of bytes, but what you want to store in the DB is also the salt & parameters, which specs almost never tell how to include with the hash. (bcrypt might be the exception)

TL;DR support for different hash formats somewhere in the future.

@paul-rouse
Copy link
Contributor

@Vlix Yes, I know! But I had hoped to be able to reproduce the hash part of the result. I am the maintainer of yesod-auth-hashdb, so had the only problem been the format of the string stored in the database, I would have been able to add something to mangle it.

However, it isn't possible, since I had failed to spot that, despite implementing PBKDF2, Yesod.Auth.Util.PasswordStore actually defaults to PBKDF1.

I am certainly not against deprecating Yesod.Auth.Util.PasswordStore, but I would need an overlap period (a year?) during which yesod-auth-hashdb continues to verify old passwords until users have changed them.

@Vlix
Copy link
Contributor Author

Vlix commented May 27, 2020

Ah, right. I also forgot about that.

It's gonna be pretty difficult changing away from PBKDF1/2 and giving the user more choice in which algorithm they'd like to use without breaking existing use cases.
Though I think it will also make it more safe, since I see a lot of Texts (and some ByteStrings) going around that could either be plain-text passwords or hashes.

If I have time (and don't forget), I might take a stab at the Yesod.Auth.Email module somewhere this summer.
@paul-rouse Once a PR is done for yesod-auth, maybe take a look and see what the best way would be to adapt yesod-auth-hashdb to the change? (To maybe modify the PR before it is accepted?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants