Page MenuHomePhabricator

Add a more modern object for storing password hashes
ClosedPublic

Authored by epriestley on Jan 21 2018, 1:23 AM.
Tags
None
Referenced Files
F13090923: D18894.diff
Thu, Apr 25, 2:38 AM
Unknown Object (File)
Sat, Apr 20, 3:31 AM
Unknown Object (File)
Fri, Apr 19, 7:06 PM
Unknown Object (File)
Fri, Apr 19, 12:00 PM
Unknown Object (File)
Sun, Apr 14, 3:48 PM
Unknown Object (File)
Tue, Apr 2, 4:46 PM
Unknown Object (File)
Tue, Apr 2, 4:45 PM
Unknown Object (File)
Tue, Apr 2, 4:45 PM
Subscribers
None

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
Branch
revoke4
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 19096
Build 25773: Run Core Tests
Build 25772: arc lint + arc unit

Event Timeline

  • Fix a typehint in the new revoker.
amckinley added inline comments.
resources/sql/autopatches/20180120.auth.02.passwordxaction.sql
11–12

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

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

What's a "test" password?

94

"associated with an object"

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

"reactivated"?

This revision is now accepted and ready to land.Jan 22 2018, 8:32 PM
resources/sql/autopatches/20180120.auth.02.passwordxaction.sql
11–12

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
19

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
27

"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.