Page MenuHomePhabricator

Add a more modern object for storing password hashes
ClosedPublic

Authored by epriestley on Jan 21 2018, 1:23 AM.

Details

Summary

Ref T13043. Currently:

  • Passwords are stored separately in the "VCS Passwords" and "User" tables and don't share as much code as they could.
  • Because User objects are all over the place in the code, password hashes are all over the place too (i.e., often somewhere in process memory). This is a very low-severity, theoretical sort of issue, but it could make leaving a stray var_dump() in the code somewhere a lot more dangerous than it otherwise is. Even if we never do this, third-party developers might. So it "feels nice" to imagine separating this data into a different table that we rarely load.
  • Passwords can not be revoked. They can be deleted, but users can set the same password again. If you believe or suspect that a password may have been compromised, you might reasonably prefer to revoke it and force the user to select a different password.

This change prepares to remedy these issues by adding a new, more modern dedicated password storage table which supports storing multiple password types (account vs VCS), gives passwords real PHIDs and transactions, supports DestructionEngine, supports revocation, and supports bin/auth revoke.

It doesn't actually make anything use this new table yet. Future changes will migrate VCS passwords and account passwords to this table.

(This also gives third party applications a reasonable place to store password hashes in a consistent way if they have some need for it.)

Test Plan

Added some basic unit tests to cover general behavior. This is just skeleton code for now and will get more thorough testing when applications move.

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 21 2018, 1:23 AM
epriestley requested review of this revision.Jan 21 2018, 1:24 AM
epriestley updated this revision to Diff 45309.Jan 21 2018, 1:43 AM
  • Fix a typehint in the new revoker.
amckinley accepted this revision.Jan 22 2018, 8:32 PM
amckinley added inline comments.
resources/sql/autopatches/20180120.auth.02.passwordxaction.sql
12–13

So these are just booleans for tracking changes to isRevoked, right?

src/applications/auth/storage/PhabricatorAuthPassword.php
20

What's a "test" password?

95

"associated with an object"

src/applications/auth/xaction/PhabricatorAuthPasswordRevokeTransaction.php
28

"reactivated"?

This revision is now accepted and ready to land.Jan 22 2018, 8:32 PM
epriestley added inline comments.Jan 22 2018, 10:28 PM
resources/sql/autopatches/20180120.auth.02.passwordxaction.sql
12–13

For now, yeah. The Transactions stuff serializes/deserializes these columns as JSON automatically, but the only possible values in this diff are "true" and "false".

After D18897 we also write transactions when hashes are upgraded (e.g., from iterated-md5 to bcrypt).

I don't have any specific plans for other types of edits/transactions here, but we could now add them if they arise. Mostly, this is just a standard template table expected by the Transactions infrastructure.

src/applications/auth/storage/PhabricatorAuthPassword.php
20

It's for unit tests (I wanted a "pure" password type for unit tests that didn't have any of the weird special cases for VCS / Account passwords).

src/applications/auth/xaction/PhabricatorAuthPasswordRevokeTransaction.php
28

"reinstated" is a little odd as the opposite of "revoked" but I wanted to avoid "reactivated" which is directly the opposite of "deactivated", which we use in other applications. This string isn't currently reachable anyway since nothing can unrevoke passwords.

I'll just update this to an explicit "removed this password from the revocation list" or something in that vein.

  • Wordsmithing ("a an", "reinstated").
This revision was automatically updated to reflect the committed changes.