Page MenuHomePhabricator

Add "bin/auth revoke --list" to explain what can be revoked
ClosedPublic

Authored by epriestley on Jan 22 2018, 5:50 PM.

Details

Summary

Depends on D18908. Ref T13043. Allow users to get information about what revokers do with a new --list flag.

You can use --list --type <key> to get information about a specfic revoker.

Test Plan

Ran bin/auth revoke --list, saw a list of revokers with useful information.

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, 5:50 PM
epriestley requested review of this revision.Jan 22 2018, 5:51 PM
amckinley accepted this revision.Jan 23 2018, 8:24 PM

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?

This revision is now accepted and ready to land.Jan 23 2018, 8:24 PM
epriestley added a comment.EditedJan 23 2018, 8:44 PM

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.

Yeah, that sounds like a reasonable approach to me. Gimmie a sec...

epriestley updated this revision to Diff 45361.Jan 23 2018, 9:16 PM
  • 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.
This revision was automatically updated to reflect the committed changes.