Details
- Reviewers
amckinley - Maniphest Tasks
- T13043: Improve authentication revocation behaviors
- Commits
- rP3becd5a57c27: Add "bin/auth revoke --list" to explain what can be revoked
Ran bin/auth revoke --list, saw a list of revokers with useful information.
Diff Detail
- Repository
- rP Phabricator
- Branch
- revoke19
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 19115 Build 25809: Run Core Tests Build 25808: arc lint + arc unit
Event Timeline
Looks good, assuming there's a good reason to revoke passwords without terminating sessions that isn't obvious to me.
| src/applications/auth/revoker/PhabricatorAuthPasswordRevoker.php | ||
|---|---|---|
| 25–27 | This feels weird... if you're responding to a possible compromise, why would you want to revoke passwords without also terminating sessions? | |
I agree that you'll usually want to revoke sessions when you revoke passwords.
Offhand, two (not-so-great) scenarios where you might not:
- You use Google OAuth, not account passwords, and are just revoking VCS passwords. They can't establish sessions so there's no reason to be more disruptive than necessary.
- The powers that be have mandated that passwords be rotated every 7 lunar months. You implement the policy like this, to minimize disruption: on Friday at 9AM, you revoke everyone's passwords. At 5PM, you revoke their sessions. During the day, they have a chance to change their passwords on their own, without disrupting their work for the most part, and come find you if there's some kind of email issue or they're just confused or they lost their MFA token. You send out email about this on Monday ("This Friday, you need to pick a new password...") and a reminder on Thursday ("Tomorrow, ..."). Thanks to your clever planning, you help a handful of users on Friday before everyone shows up at your door on Monday at 9AM complaining that they're locked out.
These scenarios are kind of made up. (In the second case, we'd be better off building a hand-holding workflow for regular cycling, probably.) Beyond that, this is just a "least surprise" sort of thing, where it feels a little magical if --revoke X also revokes (related, but distinct) credential Y.
Maybe this should really be two revokers, like vcs-password and account-password, and account-password would also revoke sessions. A downside to that is that theoretical third parties which use this infrastructure need to provide their own revokers, while with the current approach we're a little safer by default. Of course, theoretical third parties using this infrastructure are probably very theoretical.
In D18911, I tried to hedge against this error (revoking account passwords but not sessions) by providing a couple of specific examples and suggesting responses. I also think this is sort of a broader issue -- if you're revoking any of these network credentials because of any kind of general breach or concern, you probably want to nuke everything the "Network" scenario lists (conduit, password, session, temporary).
Another approach might be to provide meta-revokers, like a --type network, which revokes every credential which is sent over the network. But then third-party revokers need to have some way to say "I'm a network revoker" for whatever meta-revokers we define. Feels a little complicated for v1, maybe.
Ok, that makes sense. Maybe just make the warning about not terminating sessions a little scarier/harder to miss? Maybe after running the command, print "TAKE HEED TRAVELER, ACTIVE SESSIONS ARE STILL ACTIVE" or something similar? I can't think of a similar workflow for revoking passwords that would preserve existing sessions, so it's a little counterintuitive (and easy to miss) as-is.
- Allow revokers to provide post-run guidance.
- Guide users who run the "password" revoker toward the "session" revoker.
To test:
- Revoked passwords, got guidance.
- Revoked SSH keys, no guidance.