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

Use new PHP password hashing API #289

Merged
merged 2 commits into from
May 25, 2018
Merged

Conversation

acohn
Copy link
Contributor

@acohn acohn commented Feb 22, 2018

Currently, Plans uses the crypt() function to hash user passwords, which has the advantage of being portable but uses a somewhat weak MD5-based hash by default. PHP 5.6+/7+ includes a new API for hashing passwords that currently uses the bcrypt algorithm. It is backwards compatible with existing password hashes and will upgrade them to bcrypt as users log in.

There is one user-facing change here that will need to be called out: Users who have not changed their passwords in more than approximately five years will have passwords stored using an even more ancient DES-based algorithm, which only cares about the first eight characters of passwords. Once this PR is pushed to production, the first time users with really old passwords log in, we will remember the full password they typed. Future logins will require the same password to be typed in identically. Some users may be expecting the previous behavior, and may be surprised by the change. A MOTD will be posted calling out this change.

Additionally, this PR adds a polyfill library for the new password hashing algorithm, so Plans will maintain backwards compatibility to currently-supported PHP versions. (I think we're able to run on PHP 5.2+ currently?)

@iangreenleaf
Copy link
Member

Looks good to me. You might want to put up the MOTD for a while before deploying the change. Otherwise the message will potentially be read by people who just entered 8 characters followed by some random keys that they now cannot replicate.

Why anyone would be entering eight characters and then randomly mashing keys escapes me, but I am willing to believe that it is happening.

@acohn
Copy link
Contributor Author

acohn commented Feb 23, 2018

I will run a MOTD by the other admins today, and hope to have it posted some time this weekend.

I have heard more than one user say they randomly mash keys just to see the chroma-hash changing colors. 🤦

It would be possible to prevent this programmatically - detect a DES password hash and a submitted password that is longer than eight characters, and not update the hash - but that feels like the wrong answer.

@acohn
Copy link
Contributor Author

acohn commented May 25, 2018

We will be posting a MOTD about this change shortly. I'm merging this and will be pushing this change to production once the MOTD has been up for a bit.

@acohn acohn merged commit 649aad5 into grinnellplans:master May 25, 2018
@acohn acohn deleted the new_password_api branch May 25, 2018 17:36
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

Successfully merging this pull request may close these issues.

2 participants