Summary: Depends on D20096. Reverts D14057. This was added for Phacility use cases in D14057 but never used. It is obsoleted by {nav Auth > Customize Messages} for non-Phacility use cases.
Test Plan: Grepped for removed symbol.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20099
Summary:
Depends on D20095. Ref T13244. Currently, auth providers have a list item view and a single gigantic edit screen complete with a timeline, piles of instructions, supplemental information, etc.
As a step toward making this stuff easier to use and more modern, give them a separate view UI with normal actions, similar to basically every other type of object. Move the timeline and "Disable/Enable" to the view page (from the edit page and the list page, respectively).
Test Plan: Created, edited, and viewed auth providers.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20096
Summary:
Depends on D20094. Ref T13244. Ref T6703. See PHI774. Currently, we use an older-style radio-button UI to choose an auth provider type (Google, Password, LDAP, etc).
Instead, use a more modern click-to-select UI.
Test Plan: {F6184343}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244, T6703
Differential Revision: https://secure.phabricator.com/D20095
Summary:
Ref T13244. See PHI774. If an install does not use password auth, the "one-time login" flow (via "Welcome" email or "bin/auth recover") is pretty rough. Current behavior:
- If an install uses passwords, the user is prompted to set a password.
- If an install does not use passwords, you're dumped to `/settings/external/` to link an external account. This is pretty sketchy and this UI does not make it clear what users are expected to do (link an account) or why (so they can log in).
Instead, improve this flow:
- Password reset flow is fine.
- (Future Change) If there are external linkable accounts (like Google) and the user doesn't have any linked, I want to give users a flow like a password reset flow that says "link to an external account".
- (This Change) If you're an administrator and there are no providers at all, go to "/auth/" so you can set something up.
- (This Change) If we don't hit on any other rules, just go home?
This may be tweaked a bit as we go, but basically I want to refine the "/settings/external/" case into a more useful flow which gives users more of a chance of surviving it.
Test Plan: Logged in with passwords enabled (got password reset), with nothing enabled as an admin (got sent to Auth), and with something other than passwords enabled (got sent home).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20094
Summary: Now that we have a nice function for this, use it to simplify some code.
Test Plan: Ran through the Duo enroll workflow to make sure signing still works.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20053
Summary: Depends on D20039. Ref T13242. If installs want users to install a specific application, reference particular help, etc., let them customize the MFA enrollment message so they can make it say "if you have issues, see this walkthrough on the corporate wiki" or whatever.
Test Plan:
{F6164340}
{F6164341}
{F6164342}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20043
Summary: Depends on D20038. Ref T13231. Although I planned to keep this out of the upstream (see T13229) it ended up having enough pieces that I imagine it may need more fixes/updates than we can reasonably manage by copy/pasting stuff around. Until T5055, we don't really have good tools for managing this. Make my life easier by just upstreaming this.
Test Plan: See T13231 for a bunch of workflow discussion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231
Differential Revision: https://secure.phabricator.com/D20039
Summary: Depends on D20037. Ref T13222. Ref T7667. Although administrators can now disable MFA from the web UI, at least require that they survive MFA gates to do so. T7667 (`bin/auth lock`) should provide a sturdier approach here in the long term.
Test Plan: Created and edited MFA providers, was prompted for MFA.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T7667
Differential Revision: https://secure.phabricator.com/D20038
Summary: Depends on D20034. Ref T13222. This is just cleanup -- I thought we'd have like two of these, but we ended up having a whole lot in Duo and a decent number in SMS. Just let factors return a result explicitly if they can make a decision early. I think using `instanceof` for control flow is a lesser evil than using `catch`, on the balance.
Test Plan: `grep`, went through enroll/gate flows on SMS and Duo.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20035
Summary:
Depends on D20033. Ref T13222. Flesh this UI out a bit, and provide bit-strength information for TOTP.
Also, stop users from adding multiple SMS factors since this is pointless (they all always text your primary contact number).
Test Plan:
{F6156245}
{F6156246}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20034
Summary: Ref T13222. Providers can now be deprecated (existing factors still work, but users can't add new factors for the provider) or disabled (factors stop working, also can't add new ones).
Test Plan:
- Enabled, deprecated, and disabled some providers.
- Viewed provider detail, provider list.
- Viewed MFA settings list.
- Verified that I'm prompted for enabled + deprecated only at gates.
- Tried to disable final provider, got an error.
- Hit the MFA setup gate by enabling "Require MFA" with no providers, got a more useful message.
- Immediately forced a user to the "MFA Setup Gate" by disabling their only active provider with another provider enabled ("We no longer support TOTP, you HAVE to finish Duo enrollment to continue starting Monday.").
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20031
Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.
Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.
Test Plan: Implemented Duo, elsewhere.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D20028
Summary: Depends on D20025. Ref T13231. Although I'm not currently planning to actually upstream a Duo MFA provider, it's probably easiest to put most of the support pieces in the upstream until T5055.
Test Plan: Used a test script to make some (mostly trivial) API calls and got valid results back, so I think the parameter signing is correct.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231
Differential Revision: https://secure.phabricator.com/D20026
Summary: Depends on D20024. See D20022. Put something in place temporarily until we build out validation at some point.
Test Plan: Sent myself a test message.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20025
Summary:
Depends on D20023. Ref T13222. Although I think this isn't strictly necessary from a pure security perspective (since you can't modify the primary number while you have MFA SMS), it seems like a generally good idea.
This adds a slightly new MFA mode, where we want MFA if it's available but don't strictly require it.
Test Plan: Disabled, enabled, primaried, unprimaried, and edited contact numbers. With MFA enabled, got prompted for MFA. With no MFA, no prompts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20024
Summary:
Depends on D20022. Ref T13222. Since you can easily lock yourself out of your account by swapping to a bad number, prevent contact number edits while "contact number" MFA (today, always SMS) is enabled.
(Another approach would be to bind factors to specific contact numbers, and then prevent that number from being edited or disabled while SMS MFA was attached to it. However, I think that's a bit more complicated and a little more unwieldy, and ends up in about the same place as this. I'd consider it more strongly in the future if we had like 20 users say "I have 9 phones" but I doubt this is a real use case.)
Test Plan:
- With SMS MFA, tried to edit my primary contact number, disable it, and promote another number to become primary. Got a sensible error message in all cases.
- After removing SMS MFA, did all that stuff with no issues.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20023
Summary:
Depends on D20021. Ref T13222. This has a few rough edges, including:
- The challenges theselves are CSRF-able.
- You can go disable/edit your contact number after setting up SMS MFA and lock yourself out of your account.
- SMS doesn't require MFA so an attacker can just swap your number to their number.
...but mostly works.
Test Plan:
- Added SMS MFA to my account.
- Typed in the number I was texted.
- Typed in some other different numbers (didn't work).
- Cancelled/resumed the workflow, used SMS in conjunction with other factors, tried old codes, etc.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20022
Summary:
Depends on D20020. Ref T13222. This puts another step in the MFA enrollment flow: pick a provider; read text and click "Continue"; actually enroll.
This is primarily to stop CSRF attacks, since otherwise an attacker can put `<img src="phabricator.com/auth/settings/enroll/?providerPHID=xyz" />` on `cute-cat-pix.com` and get you to send yourself some SMS enrollment text messages, which would be mildly annoying.
We could skip this step if we already have a valid CSRF token (and we often will), but I think there's some value in doing it anyway. In particular:
- For SMS/Duo, it seems nice to have an explicit "we're about to hit your phone" button.
- We could let installs customize this text and give users a smoother onboard.
- It allows the relatively wordy enroll form to be a little less wordy.
- For tokens which can expire (SMS, Duo) it might save you from answering too slowly if you have to go dig your phone out of your bag downstairs or something.
Test Plan: Added factors, read text. Tried to CSRF the endpoint, got a dialog instead of a live challenge generation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20021
Summary:
Depends on D20019. Ref T13222. Currently, TOTP uses a temporary token to make sure you've set up the app on your phone properly and that you're providing an answer to a secret which we generated (not an attacker-generated secret).
However, most factor types need some kind of sync token. SMS needs to send you a code; Duo needs to store a transaction ID. Turn this "TOTP" token into an "MFA Sync" token and lift the implementation up to the base class.
Also, slightly simplify some of the HTTP form gymnastics.
Test Plan:
- Hit the TOTP enroll screen.
- Reloaded it, got new secrets.
- Reloaded it more than 10 times, got told to stop generating new challenges.
- Answered a challenge properly, got a new TOTP factor.
- Grepped for removed class name.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20020
Summary:
Depends on D20018. Ref T13222. When you add a new MFA configuration, you can technically (?) guess your way through it with brute force. It's not clear why this would ever really be useful (if an attacker can get here and wants to add TOTP, they can just add TOTP!) but it's probably bad, so don't let users do it.
This limit is fairly generous because I don't think this actually part of any real attack, at least today with factors we're considering.
Test Plan:
- Added TOTP, guessed wrong a ton of times, got rate limited.
- Added TOTP, guessed right, got a TOTP factor configuration added to my account.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20019
Summary:
Depends on D20017. Ref T13222. Currently, if you:
- type some text at a TOTP gate;
- wait ~60 seconds for the challenge to expire;
- submit the form into a "Wait patiently" message; and
- mash that wait button over and over again very patiently
...you still rack up rate limiting points, because the hidden text from your original request is preserved and triggers the "is the user responding to a challenge" test. Only perform this test if we haven't already decided that we're going to make them wait.
Test Plan:
- Did the above; before patch: rate limited; after patch: not rate limited.
- Intentionally typed a bunch of bad answers which were actually evaluated: rate limited properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20018
Summary:
Depends on D20016. Ref T920. This does nothing interesting on its own since the TOTP provider has no guidance/warnings, but landing it separately helps to simplify an upcoming SMS diff.
SMS will have these guidance messages:
- "Administrator: you haven't configured any mailer which can send SMS, like Twilio."
- "Administrator: SMS is weak."
- "User: you haven't configured a contact number."
Test Plan: {F6151283} {F6151284}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20017
Summary:
Depends on D20011. Ref T920. This change lets a "MetaMTAMail" storage object represent various different types of messages, and makes "all" the `bin/mail` stuff "totally work" with messages of non-email types.
In practice, a lot of the related tooling needs some polish/refinement, but the basics work.
Test Plan: Used `echo beep boop | bin/mail send-test --to epriestley --type sms` to send myself SMS.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20012
Summary:
Depends on D20010. Ref T920. Allow users to designate which contact number is "primary": the number we'll actually send stuff to.
Since this interacts in weird ways with "disable", just do a "when any number is touched, put all of the user's rows into the right state" sort of thing.
Test Plan:
- Added numbers, made numbers primary, disabled a primary number, un-disabled a number with no primaries. Got sensible behavior in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20011
Summary: Depends on D20008. Ref T920. Continue fleshing out contact number behaviors.
Test Plan:
- Enabled and disabled a contact number.
- Saw list, detail views reflect change.
- Added number X, disabled it, added it again (allowed), enabled the disabled one ("already in use" exception).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20010
Summary:
Depends on D20006. Ref T13222. Currently, the "MFA Is Required" gate doesn't let you do anything else, but you'll need to be able to access "Contact Numbers" if an install provides SMS MFA.
Tweak this UI to give users limited access to settings, so they can set up contact numbers and change their language.
(This is a little bit fiddly, and I'm doing it early on partly so it can get more testing as these changes move forward.)
Test Plan: {F6146136}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20008
Summary:
Ref T920. To send you SMS messages, we need to know your phone number.
This adds bare-bone basics (transactions, storage, editor, etc).
From here:
**Disabling Numbers**: I'll let you disable numbers in an upcoming diff.
**Primary Number**: I think I'm just going to let you pick a number as "primary", similar to how email works. We could imagine a world where you have one "MFA" number and one "notifications" number, but this seems unlikely-ish?
**Publishing Numbers (Profile / API)**: At some point, we could let you say that a number is public / "show on my profile" and provide API access / directory features. Not planning to touch this for now.
**Non-Phone Numbers**: Eventually this could be a list of other similar contact mechanisms (APNS/GCM devices, Whatsapp numbers, ICQ number, twitter handle so MFA can slide into your DM's?). Not planning to touch this for now, but the path should be straightforward when we get there. This is why it's called "Contact Number", not "Phone Number".
**MFA-Required + SMS**: Right now, if the only MFA provider is SMS and MFA is required on the install, you can't actually get into Settings to add a contact number to configure SMS. I'll look at the best way to deal with this in an upcoming diff -- likely, giving you partial access to more of Setings before you get thorugh the MFA gate. Conceptually, it seems reasonable to let you adjust some other settings, like "Language" and "Accessibility", before you set up MFA, so if the "you need to add MFA" portal was more like a partial Settings screen, maybe that's pretty reasonable.
**Verifying Numbers**: We'll probably need to tackle this eventually, but I'm not planning to worry about it for now.
Test Plan: {F6137174}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: avivey, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19988
Summary:
Ref T13222. This updates the CLI tools and documentation for the changes in D19975.
The flags `--type` and `--all-types` retain their current meaning. In most cases, `bin/auth strip --type totp` is sufficient and you don't need to bother looking up the relevant provider PHID. The existing `bin/auth list-factors` is also unchanged.
The new `--provider` flag allows you to select configs from a particular provider in a more granular way. The new `bin/auth list-mfa-providers` provides an easy way to get PHIDs.
(In the Phacility cluster, the "Strip MFA" action just reaches into the database and deletes rows manually, so this isn't terribly important. I verified that the code should still work properly.)
Test Plan:
- Ran `bin/auth list-mfa-providers`.
- Stripped by user / type / provider.
- Grepped for `list-factors` and `auth strip`.
- Hit all (?) of the various possible error cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19976
Summary:
Ref T13222. Users configure "Factor Configs", which say "I have an entry on my phone for TOTP secret key XYZ".
Currently, these point at raw implementations -- always "TOTP" in practice.
To support configuring available MFA types (like "no MFA") and adding MFA types that need some options set (like "Duo", which needs API keys), bind "Factor Configs" to a "Factor Provider" instead.
In the future, several "Factors" will be available (TOTP, SMS, Duo, Postal Mail, ...). Administrators configure zero or more "MFA Providers" they want to use (e.g., "Duo" + here's my API key). Then users can add configs for these providers (e.g., "here's my Duo account").
Upshot:
- Factor: a PHP subclass, implements the technical details of a type of MFA factor (TOTP, SMS, Duo, etc).
- FactorProvider: a storage object, owned by administrators, configuration of a Factor that says "this should be available on this install", plus provides API keys, a human-readable name, etc.
- FactorConfig: a storage object, owned by a user, says "I have a factor for provider X on my phone/whatever with secret key Q / my duo account is X / my address is Y".
Couple of things not covered here:
- Statuses for providers ("Disabled", "Deprecated") don't do anything yet, but you can't edit them anyway.
- Some `bin/auth` tools need to be updated.
- When no providers are configured, the MFA panel should probably vanish.
- Documentation.
Test Plan:
- Ran migration with providers, saw configs point at the first provider.
- Ran migration without providers, saw a provider created and configs pointed at it.
- Added/removed factors and providers. Passed MFA gates. Spot-checked database for general sanity.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19975
Summary: See https://secure.phabricator.com/D18901#249481. Update the docs and a warning string to reflect the new reality that `bin/auth recover` is now able to recover any account, not just administrators.
Test Plan: Mk 1 eyeball
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20007
Summary:
Depends on D19994. See PHI1027. If an install has customized the "Welcome Mail" message, include it in welcome mail. A special custom message from the profile screen overrides it, if provided.
(I fiddled with putting the custom message as "placeholder" text in the remarkup area as a hint, but newlines in "placeholder" text appear to have issues in Safari and Firefox. I think this is probably reasonably clear as-is.)
Make both render remarkup-into-text so things like links work properly, as it's reasonably likely that installs will want to link to things.
Test Plan:
- With custom "Welcome Mail" text, sent mail with no custom override (got custom text) and a custom override (got overridden text).
- Linked to some stuff, got sensible links in the mail (`bin/mail show-outbound`).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19995
Summary: Depends on D19992. Ref T13222. If administrators provide a custom login message, show it on the login screen.
Test Plan:
{F6137930}
- Viewed login screen with and without a custom message.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19994
Summary:
Ref T13222. Long ago, we had a Config option (`welcome.html`) to let you dump HTML onto the login screen, but this was relatively hard to use and not good from a security perspective.
In some cases this was obsoleted by Dashboards, but there's at least some remaining set of use cases for actual login instructions on the login screen. For example, WMF has some guidance on //which// SSO mechanism to use based on what types of account you have. On `secure`, users assume they can register by clicking "Log In With GitHub" or whatever, and it might reduce frustration to tell them upfront that registration is closed.
Some other types of auth messaging could also either use customization or defaults (e.g., the invite/welcome/approve mail).
We could do this with a bunch of Config options, but I'd generally like to move to a world where there's less stuff in Config and more configuration is contextual. I think it tends to be easier to use, and we get a lot of fringe benefits (granular permissions, API, normal transaction logs, more abililty to customize workflows and provide contextual help/hints, etc). Here, for example, we can provide a remarkup preview, which would be trickier with Config.
This does not actually do anything yet.
Test Plan: {F6137541}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19992
Summary:
Ref T13222. Currently, if a remote address fails a few login attempts (5) in a short period of time (15 minutes) we require a CAPTCHA for each additional attempt.
This relies on:
- Administrators configuring ReCAPTCHA, which they may just not bother with.
- Administrators being comfortable with Google running arbitrary trusted Javascript, which they may not be comfortable with.
- ReCAPTCHA actually being effective, which seems likely true for unsophisticated attackers but perhaps less true for more sophisticated attackers (see <https://github.com/ecthros/uncaptcha2>, for example).
(For unsophisticated attackers and researchers, "Rumola" has been the standard CAPTCHA bypass tool for some time. This is an extension that pays humans to solve CAPTCHAs for you. This is not practical at "brute force a strong password" scale. Google appears to have removed it from the Chrome store. The "submit the captcha back to Google's APIs" trick probably isn't practical at brute-force-scale either, but it's easier to imagine weaponizing that than weaponizing human solvers.)
Add a hard gate behind the CAPTHCA wall so that we fail into a secure state if there's no CAPTCHA or the attacker can defeat CAPTCHAs at a very low cost.
The big downside to this is that an attacker who controls your remote address (e.g., is behind the same NAT device you're behind on corpnet) can lock you out of your account. However:
- That //should// be a lot of access (although maybe this isn't that high of a barrier in many cases, since compromising a "smart fridge" or "smart water glass" or whatever might be good enough).
- You can still do "Forgot password?" and login via email link, although this may not be obvious.
Test Plan:
- Logged in normally.
- Failed many many login attempts, got hard gated.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19997
Summary:
Ref T13222. Ref T13231. See PHI912. I'm planning to turn MFA providers into concrete objects, so you can disable and configure them.
Currently, we only support TOTP, which doesn't require any configuration, but other provider types (like Duo or Yubikey OTP) do require some configuration (server URIs, API keys, etc). TOTP //could// also have some configuration, like "bits of entropy" or "allowed window size" or whatever, if we want.
Add concrete objects for this and standard transaction / policy / query support. These objects don't do anything interesting yet and don't actually interact with MFA, this is just skeleton code for now.
Test Plan:
{F6090444}
{F6090445}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D19935
Summary:
Ref T12509.
- Remove the "phabricator.csrf-key" configuration option in favor of automatically generating an HMAC key.
- Upgrade two hasher callsites (one in CSRF itself, one in providing a CSRF secret for logged-out users) to SHA256.
- Extract the CSRF logic from `PhabricatorUser` to a standalone engine.
I was originally going to do this as two changes (extract logic, then upgrade hashes) but the logic had a couple of very silly pieces to it that made faithful extraction a little silly.
For example, it computed `time_block = (epoch + (offset * cycle_frequency)) / cycle_frequency` instead of `time_block = (epoch / cycle_frequency) + offset`. These are equivalent but the former was kind of silly.
It also computed `substr(hmac(substr(hmac(secret)).salt))` instead of `substr(hmac(secret.salt))`. These have the same overall effect but the former is, again, kind of silly (and a little bit materially worse, in this case).
This will cause a one-time compatibility break: pages loaded before the upgrade won't be able to submit contained forms after the upgrade, unless they're open for long enough for the Javascript to refresh the CSRF token (an hour, I think?). I'll note this in the changelog.
Test Plan:
- As a logged-in user, submitted forms normally (worked).
- As a logged-in user, submitted forms with a bad CSRF value (error, as expected).
- As a logged-out user, hit the success and error cases.
- Visually inspected tokens for correct format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19946
Summary:
Depends on D19906. Ref T13222. This isn't going to win any design awards, but make the "wait" and "answered" elements a little more clear.
Ideally, the icon parts could be animated Google Authenticator-style timers (but I think we'd need to draw them in a `<canvas />` unless there's some clever trick that I don't know) or maybe we could just have the background be like a "water level" that empties out. Not sure I'm going to actually write the JS for either of those, but the UI at least looks a little more intentional.
Test Plan:
{F6070914}
{F6070915}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19908
Summary:
Depends on D19905. Ref T13222. In D19843, I refactored this stuff but `$jump_into_hisec` was dropped.
This is a hint to keep the upgraded session in hisec mode, which we need to do a password reset when using a recovery link. Without it, we double prompt you for MFA: first to upgrade to a full session, then to change your password.
Pass this into the engine properly to avoid the double-prompt.
Test Plan:
- Used `bin/auth recover` to get a partial session with MFA enabled and a password provider.
- Before: double MFA prompt.
- After: session stays upgraded when it becomes full, no second prompt.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19906
Summary:
Depends on D19904. Ref T13226. Ref T13222. Currently, partial sessions (where you've provided a primary auth factor like a password, but not yet provided MFA) work like normal sessions: they're good for 30 days and extend indefinitely under regular use.
This behavior is convenient for full sessions, but normal users don't ever spend 30 minutes answering MFA, so there's no real reason to do it for partial sessions. If we add login alerts in the future, limiting partial sessions to a short lifetime will make them more useful, since an attacker can't get one partial session and keep extending it forever while waiting for an opportunity to get past your MFA.
Test Plan:
- Did a partial login (to the MFA prompt), checked database, saw a ~29 minute partial session.
- Did a full login, saw session extend to ~30 days.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13226, T13222
Differential Revision: https://secure.phabricator.com/D19905
Summary:
Depends on D19903. Ref T13222. This was a Facebook-specific thing from D6202 that I believe no other install ever used, and I'm generally trying to move away from the old "event" system (the more modern modular/engine patterns generally replace it).
Just drop support for this. Since the constant is being removed, anything that's actually using it should break in an obvious way, and I'll note this in the changelog.
There's no explicit replacement but I don't think this hook is useful for anything except "being Facebook in 2013".
Test Plan:
- Grepped for `TYPE_AUTH_WILLLOGIN`.
- Logged in.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19904
Summary:
Depends on D19902. Ref T13222. This is mostly a "while I'm in here..." change since MFA is getting touched so much anyway.
Doing cluster support, I sometimes need to log into user accounts on instances that have MFA. I currently accomplish this by doing `bin/auth recover`, getting a parital session, and then forcing it into a full session in the database. This is inconvenient and somewhat dangerous.
Instead, allow `bin/auth recover` to generate a link that skips the "partial session" stage: adding required MFA, providing MFA, and signing legalpad documents.
Anyone who can run `bin/auth recover` can do this anyway, this just reduces the chance I accidentally bypass MFA on the wrong session when doing support stuff.
Test Plan:
- Logged in with `bin/auth recover`, was prompted for MFA.
- Logged in with `bin/auth recover --force-full-session`, was not prompted for MFA.
- Did a password reset, followed reset link, was prompted for MFA.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19903
Summary:
Depends on D19899. Ref T13222. When we prompt you for one-shot MFA, we currently give you a lot of misleading text about your session staying in "high security mode".
Differentiate between one-shot and session upgrade MFA, and give the user appropriate cues and explanatory text.
Test Plan:
- Hit one-shot MFA on an "mfa" task in Maniphest.
- Hit session upgrade MFA in Settings > Multi-Factor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19900
Summary:
Depends on D19897. Ref T13222. See some discussion in D19890.
- Only rate limit users if they're actually answering a challenge, not if they're just clicking "Wait Patiently".
- Reduce the number of allowed attempts per hour from 100 back to 10.
- Reduce the TOTP window from +/- 2 timesteps (allowing ~60 seconds of skew) to +/- 1 timestep (allowing ~30 seconds of skew).
- Change the window where a TOTP response remains valid to a flat 60 seconds instead of a calculation based on windows and timesteps.
Test Plan:
- Hit an MFA prompt.
- Without typing in any codes, mashed "submit" as much as I wanted (>>10 times / hour).
- Answered prompt correctly.
- Mashed "Wait Patiently" as much as I wanted (>>10 times / hour).
- Guessed random numbers, was rate limited after 10 attempts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19898
Summary:
Depends on D19896. Ref T13222. See PHI873. Add a core "Sign With MFA" transaction type which prompts you for MFA and marks your transactions as MFA'd.
This is a one-shot gate and does not keep you in MFA.
Test Plan:
- Used "Sign with MFA", got prompted for MFA, answered MFA, saw transactions apply with MFA metadata and markers.
- Tried to sign alone, got appropriate errors.
- Tried to sign no-op changes, got appropriate errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19897
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary:
Depends on D19894. Ref T13222. See PHI873. When you provide a correct response to an MFA challenge, we mark it as "answered".
Currently, we never let you reuse an "answered" token. That's usually fine, but if you have 2+ factors on your account and get one or more (but fewer than all of them) right when you submit the form, you need to answer them all again, possibly after waiting for a lockout period. This is needless.
When you answer a challenge correctly, add a hidden input with a code proving you got it right so you don't need to provide another answer for a little while.
Why not just put your response in a form input, e.g. `<input type="hidden" name="totp-response" value="123456" />`?
- We may allow the "answered" response to be valid for a different amount of time than the actual answer. For TOTP, we currently allow a response to remain valid for 60 seconds, but the actual code you entered might expire sooner.
- In some cases, there's no response we can provide (with push + approve MFA, you don't enter a code, you just tap "yes, allow this" on your phone). Conceivably, we may not be able to re-verify a push+approve code if the remote implements one-shot answers.
- The "responseToken" stuff may end up embedded in normal forms in some cases in the future, and this approach just generally reduces the amount of plaintext MFA we have floating around.
Test Plan:
- Added 2 MFA tokens to my account.
- Hit the MFA prompt.
- Provided one good response and one bad response.
- Submitted the form.
- Old behavior: good response gets locked out for ~120 seconds.
- New behavior: good response is marked "answered", fixing the other response lets me submit the form.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19895
