Page MenuHomePhabricator

Improve authentication revocation behaviors
Closed, ResolvedPublic

Description

See PHI304. Requests to cycle/revoke credentials have been rare and we have limited support for automating it, but should improve support.

Currently, we have bin/auth revoke but it supports only Conduit revocations. This should be expanded; specifically:

  • Add a temporary token revoker.
  • Add a session revoker.
  • Add an SSH key revoker.

Then:

  • Password hashes are stored on the user object. VCS password hashes are stored in RepositoryVCSPassword. These would probably be better if moved to a central password table with a role column.
  • Passwords can be removed, but can not be revoked (i.e., forcing users to change passwords). This table should also be able to store revoked passwords.
  • Add a password revoker.
  • Add a VCS password revoker.

Additionally (see T7148):

  • When instances are exported from Phacility, we currently do not strip VCS passwords, but should.
  • When instances are exported from Phacility, we currently do not strip Conduit tokens, but should.
  • Ideally, this workflow should just use bin/auth revoke to reduce code duplication.

Errata:

  • Is AuthTemporaryToken->userPHID used by anything?
  • VCS passwords should respect account.minimum-password-length.
  • While I'm here, we could add a rate limit to "Change Password" to stop occasional researcher reports about this.
  • When SSH keys are revoked with bin/auth revoke, the email should probably exclude the "your account may have been compromised" warning.
  • Write some documentation.
  • Carve a pathway forward on the legacy digest algorithms.

Details

Commits
D18929 / rP5529458e14eb: Add test coverage for SSH key revocation
D18928 / rPdeb754dfe173: Make SSH key revocation actually prevent adding the same key back
D18916 / rPbf2868c07016: Rename "PhabricatorPasswordHashInterface" to…
D18911 / rPd4b3cd5255b6: Document the "bin/auth revoke" tool
D18910 / rP3becd5a57c27: Add "bin/auth revoke --list" to explain what can be revoked
D18908 / rP21e415299f26: Mark all existing password hashes as "legacy" and start upgrading digest formats
D18907 / rP13ef5c6f2314: When administrators revoke SSH keys, don't include a "security warning" in the…
D18906 / rP026ec11b9d6b: Add a rate limit for guessing old passwords when changing passwords
Restricted Differential Revision / Restricted Diffusion Commit
D18904 / rPcab2bba6f2f7: Remove "passwordHash" and "passwordSalt" from User objects
D18903 / rPabc030fa008b: Move account passwords to shared infrastructure
D18902 / rPb8a515cb29d8: Bring new password validation into AuthPasswordEngine
D18901 / rPaa3b582c7b72: Remove "set password" from `bin/accountadmin` and let `bin/auth recover`…
D18900 / rP5a8a56f414fc: Prepare the new AuthPassword infrastructure for storing account passwords
D18899 / rP753c4c5ff1ad: Remove the "PhabricatorRepositoryVCSPassword" class and table
D18898 / rPdd8f588ac51b: Migrate VCS passwords to new shared password infrastructure
D18897 / rPbb12f4bab718: Add test coverage to the PasswordEngine upgrade workflow and fix a few bugs
D18896 / rPc280c85772a3: Consolidate password verification/revocation logic in a new…
D18895 / rP42e2cd9af02e: Add a "--force" flag to `bin/auth revoke`
D18894 / rP9c00a437848c: Add a more modern object for storing password hashes
D18893 / rPfa1ecb7f6612: Add a `bin/auth revoke` revoker for SSH keys
D18892 / rP39c3b10a2f21: Add a `bin/auth revoke` revoker for sessions
D18891 / rP7970cf058517: Add a `bin/auth revoke` revoker for temporary tokens

Event Timeline

epriestley triaged this task as Normal priority.

See D17458 for the previous, narrower case of token revocation in response to Heartbleed (T12313).

epriestley updated the task description. (Show Details)Jan 20 2018, 4:52 PM
epriestley updated the task description. (Show Details)Jan 21 2018, 11:16 PM
epriestley updated the task description. (Show Details)Jan 22 2018, 12:50 AM
epriestley updated the task description. (Show Details)Jan 22 2018, 12:53 AM
epriestley updated the task description. (Show Details)Jan 22 2018, 1:20 AM
epriestley added a revision: Restricted Differential Revision.Jan 22 2018, 1:34 AM
epriestley closed this task as Resolved.Jan 23 2018, 11:43 PM

Add a temporary token revoker.
Add a session revoker.
Add an SSH key revoker.
Add a password revoker.
Add a VCS password revoker.

These have been added.

Password hashes are stored on the user object. VCS password hashes are stored in RepositoryVCSPassword. These would probably be better if moved to a central password table with a role column.

VCS and account passwords are now stored in PhabricatorAuthPassword and share much more code.

User objects no longer contain password hashes or salts in object properties, theoretically reducing the risk of this material leaking via stack traces and debugging mechanisms.

Passwords can be removed, but can not be revoked (i.e., forcing users to change passwords). This table should also be able to store revoked passwords.

Passwords can now be revoked so that users may not select the same password again.

When instances are exported from Phacility, we currently do not strip VCS passwords, but should.
When instances are exported from Phacility, we currently do not strip Conduit tokens, but should.
Ideally, this workflow should just use bin/auth revoke to reduce code duplication.

We now strip all this stuff and use bin/auth revoke where it makes sense (pending an actual production test).

Is AuthTemporaryToken->userPHID used by anything?

Yes, LFS uses this. This column name is slightly confusing but since it has a legitimate callsite I'm keeping this pecularity out of scope.

VCS passwords should respect account.minimum-password-length.

All password controllers now use the same validation logic and respect account.minimum-password-length (D18902).

While I'm here, we could add a rate limit to "Change Password" to stop occasional researcher reports about this.

This action limit has been installed (D18906).

When SSH keys are revoked with bin/auth revoke, the email should probably exclude the "your account may have been compromised" warning.

This warning is no longer included in the related mail (D18907).

Write some documentation.

Penned thusly (D18911).

Carve a pathway forward on the legacy digest algorithms.

New VCS and Account passwords are digested in a modern, simple way with HMAC SHA256 and a password-specific salt.

Old passwords are automatically upgraded the next time they are used. This is similar to the existing logic to upgrade hashers if a better hasher becomes available.

Old passwords that are never used can not be automatically upgraded via migration (we can't reverse the hashes). For now, we've versioned them and retained the older digest logic so they'll continue working. We may remove this logic and break these credentials at some point far in the future. If we do, users will need to reset any passwords they haven't used in a year or some similar timeframe.

Broadly, with the minor exception of the legacy digests, I think our password infrastructure no longer has any unusual components or odd technical choices. It was more or less satisfactory before and I think we got all the stuff with any real security impact right, there were just a few weird bits -- like usernames as salt for passwords, so that changing usernames broke passwords -- which are no longer weird.