Page MenuHomePhabricator

Mark all existing password hashes as "legacy" and start upgrading digest formats
ClosedPublic

Authored by epriestley on Jan 22 2018, 2:16 AM.

Details

Summary

Depends on D18907. Ref T13043. Ref T12509. We have some weird old password digest behavior that isn't terribly concerning, but also isn't great.

Specifically, old passwords were digested in weird ways before being hashed. Notably, account passwords were digested with usernames, so your password stops working if your username is chagned. Not the end of the world, but silly.

Mark all existing hashes as "v1", and automatically upgrade then when they're used or changed. Some day, far in the future, we could stop supporting these legacy digests and delete the code and passwords and just issue upgrade advice ("Passwords which haven't been used in more than two years no longer work."). But at least get things on a path toward sane, modern behavior.

Test Plan

Ran migration. Spot-checked that everthing in the database got marked as "v1". Used an existing password to login successfully. Verified that it was upgraded to a null (modern) digest. Logged in with it again.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 22 2018, 2:16 AM
epriestley requested review of this revision.Jan 22 2018, 2:17 AM
epriestley updated this revision to Diff 45326.Jan 22 2018, 2:18 AM
  • Fix some grammar and spelling.

It feels a little weird to create this column, only to null it out as soon as possible. I get that we can't upgrade the digest format until the next time a user logs in, but I wonder if there's a better way to handle this. Maybe rename legacyDigestFormat to just digestFormat, and set the column to v2 or something when we upgrade the format?

src/applications/people/storage/PhabricatorUser.php
1634–1639

Is the actual business-end of this getting updated in a different revision? Looks like it's going to be out of sync with the comment block above it.

I agree that this is a little weird. This way (where legacyDigestFormat is like pendingRequiredDigestFormatMigration) seemed maybe a little cleaner to me because it's easier to throw away later.

In particular, anything else which uses this table (e.g., third party code) can just ignore the digest format column completely and pretend it doesn't exist. It doesn't need to store a dummy v1 or anything. Then, if we delete it in the future, that code won't break.

If we require a real value in the column, the logic needs to get a little more complicated and additional implementations need to return a real digest version.

Of course, I'm not sure there will ever be any other callers for this table (how many third-party applications need to store password hashes?) but even if there aren't this code is a bit easier to get rid of when structured like this: we basically just revert this change and delete the special logic and we're done. If this method returned a <version, digest> pair we'd need to do a (small) amount of additional work to get rid of the version code completely.

Specifically, the additional complexity arises when we hash a new password and need to figure out which "real" digest version to store. We could return a <version, digest> pair from this method instead of just a digest, or we could add some method like getCurrentPasswordDigestVersion(). Both seemed a little more complicated than just treating this column as pendingMigration, though?

src/applications/people/storage/PhabricatorUser.php
1634–1639

Out of sync in the sense of the comment saying stuff like "previously used" even though the code still exists and is reachable?

The block is like "we used to do this nonsense, which is why we have this weird piece of code here for old passwords", if that makes sense?

I'm also making these guesses about future complexity with the assumption that the HMAC SHA256 approach will be a very good choice for a very long time. I think this is likely to be true, since it doesn't have any baggage and SHA256 is a modern hash.

amckinley accepted this revision.Jan 23 2018, 8:30 PM

Cool, that explanation makes sense. I agree it's unlikely that we'll need to upgrade the hash format again for Many Moons™.

src/applications/people/storage/PhabricatorUser.php
1634–1639

Oh, that makes more sense.

This revision is now accepted and ready to land.Jan 23 2018, 8:30 PM
This revision was automatically updated to reflect the committed changes.