Page MenuHomePhabricator

2018 Week 48-51 Bonus Content
Closed, ResolvedPublic

Description

See PHI873, which opens a discussion around broader authentication and signatures.

See PHI857, which wants support for serial queues in repository operations.

See PHI989, which notes some consistency issues with certain datasource queries, particularly when some components are empty.


Clear!

See PHI986 and PHI896, which would both like more Harbormaster API methods.

See PHI992, which has some clustering followups: more flags for bin/repository thaw; Service API support for repositories; a greater swath of destruction for Interfaces.

See PHI990: user.search should support availability information (busy/away).

See PHI683, which would like "Change Task Subtype..." to become first-class (particularly, a selectable comment action) and for "Create Subtask" to allow selection of task subtypes (see T12588).

See D19829 for a followup change: getDetail()/setDetail() should be more-private APIs than they are today, and callers elsewhere (like transaction logic) should use methods like getEncoding(), not getDetail('encoding').

See PHI995. Previous changes left a pointless "<author> marked 6 [of their own] inlines as done." in timelines and email. This transaction should just be hidden.

See PHI996. This is T10743. Test notifications currently broadcast into feed globally, but they should be scoped down to the testing user and should not appear in feed.

See PHI994, which needs some exploration.

Details

Commits
D20038 / rPd8d4efe89e85: Require MFA to edit MFA providers
D20037 / rP44a0b3e83d90: Replace "Show Secret" in Passphrase with one-shot MFA
D20036 / rPd24e66724d3b: Convert "Rename User" from session MFA to one-shot MFA
D20035 / rP29b4fad94173: Get rid of "throwResult()" for control flow in MFA factors
D20034 / rPbce44385e1e3: Add more factor details to the Settings factor list
D20033 / rP2dd8a0fc6925: Update documentation for MFA, including administrator guidance
D20032 / rP50abc873630b: Expand outbound mailer documentation to mention SMS and include Twilio
D20031 / rP8e5d9c6f0eb5: Allow MFA providers to be deprecated or disabled
D20028 / rPc9ff6ce390d6: Add CSRF to SMS challenges, and pave the way for more MFA types (including Duo)
D20024 / rP587e9cea19ac: Always require MFA to edit contact numbers
D20023 / rP7805b217ad8b: Prevent users from editing, disabling, or swapping their primary contact number…
D20022 / rPada8a56bb7db: Implement SMS MFA
D20021 / rP6c11f373965c: Add a pre-enroll step for MFA, primarily as a CSRF gate
D20020 / rPf3340c633562: Allow different MFA factor types (SMS, TOTP, Duo, ...) to share "sync" tokens…
D20019 / rP7c1d1c13f4a3: Add a rate limit for enroll attempts when adding new MFA configurations
D20018 / rPe91bc26da685: Don't rate limit users clicking "Wait Patiently" at an MFA gate even if they…
D20008 / rPc4244aa177b8: Allow users to access some settings at the "Add MFA" account setup roadblock
D19976 / rPaa483738899d: Update `bin/auth` MFA commands for the new "MFA Provider" indirection layer
D19975 / rP0fcff782533a: Convert user MFA factors to point at configurable "MFA Providers", not raw "MFA…
D19994 / rP22ad1ff2c5b1: Show the customized "Login" message on the login screen
D19992 / rP2c713b2d25fd: Add "Auth Messages" to support customizing onboarding/welcome flows
D19997 / rP310ad7f8f47b: Put a hard limit on password login attempts from the same remote address
D19987 / rPff220acae6a4: Don't bounce mail messages if any recipient was reserved
D19965 / rPc3cafffed726: Update the "SES" and "sendmail" mailers for the new API; remove "encoding"
D19953 / rPa37b28ef79cb: Prevent inbound processing of the "void/placeholder" address and other reserved…
D19935 / rPa62f334d9503: Add a skeleton for configurable MFA provider types
D19981 / rPHUc424488c6cf0: Use "random_bytes()" under newer PHP, and introduce "Filesystem…
D19943 / rP3963c86ad5e5: Pass timeline view data to comment previews, restoring Differential comment…
D19909 / rP106e90dcf086: Remove the "willApplyTransactions()" hook from ApplicationTransactionEditor
D19908 / rP1729e7b46792: Improve UI for "wait" and "answered" MFA challenges
D19906 / rP918f4ebcd82c: Fix a double-prompt for MFA when recovering a password account
D19905 / rPca39be60914b: Make partial sessions expire after 30 minutes, and do not extend them
D19904 / rP38c48ae7d048: Remove support for the "TYPE_AUTH_WILLLOGIN" event
D19903 / rPff49d1ef776b: Allow "bin/auth recover" to generate a link which forces a full login session
D19902 / rP6a6db0ac8e6f: Allow tokens to be awarded to MFA-required objects
D19901 / rPefb01bf34f60: Allow "MFA Required" objects to be edited without MFA if the edit is only…
D19900 / rP1c89b3175f1c: Improve UI messaging around "one-shot" vs "session upgrade" MFA
D19899 / rPd3c325c4fc73: Allow objects to be put in an "MFA required for all interactions" mode, and…
D19898 / rP3da9844564cf: Tighten some MFA/TOTP parameters to improve resistance to brute force attacks
D19897 / rP543f2b6bf156: Allow any transaction group to be signed with a one-shot "Sign With MFA" action
Restricted Differential Revision / Restricted Diffusion Commit
D19896 / rP961fd7e8491e: In Legalpad, prompt for MFA at the end of the workflow instead of the beginning
D19895 / rPb63783c06718: Carry MFA responses which have been "answered" but not "completed" through the…
D19894 / rPce953ea44790: Explicitly mark MFA challenges as "answered" and "completed"
D19893 / rP657f3c380608: When accepting a TOTP response, require it respond explicitly to a specific…
D19890 / rP0673e79d6d96: Simplify and correct some challenge TTL lockout code
D19889 / rP46052878b1de: Bind MFA challenges to particular workflows, like signing a specific Legalpad…
D19888 / rP5e94343c7d1a: Add a garbage collector for MFA challenges
D19886 / rPb8cbfda07ce6: Track MFA "challenges" so we can bind challenges to sessions and support SMS…
D19885 / rPc731508d748a: Require MFA implementations to return a formal result object when validating…
D19884 / rP080fb1985f29: Upgrade an old "weakDigest()" inside TOTP synchronization code
D19883 / rP1d34238dc945: Upgrade sessions digests to HMAC256, retaining compatibility with old digests
D19881 / rPc58506aeaace: Give sessions real PHIDs and slightly modernize session queries
D19882 / rPHUcad1985726c9: Fix construction of two new qsprintf() exceptions
D19867 / rP0e067213fbb1: Make viewing a user's profile page clear notifications about that user
D19866 / rP05900a4cc990: Add a CLI workflow for testing that notifications are being delivered
D19865 / rPe43f9124f8d4: Remove obsolete "NotifyTest" feed story
D19864 / rP773b4eaa9ea0: Separate "feed" and "notifications" better, allow stories to appear in…
D19861 / rPba833805656e: Update the "Notification Test" workflow to use more modern mechanisms
D19860 / rP55a1ef339f45: Fix a bad method call signature throwing exceptions in newer Node
D19859 / rP508df60a6217: When users mark their own inline comments as "Done", suppress the timeline/mail…
D19858 / rP46feccdfcf19: Share more inline "Done" code between Differential and Diffusion
D19855 / rP68b1dee1390d: Replace the "Choose Subtype" radio buttons dialog with a simpler "big stuff you…
D19854 / rPa6632f8c188a: Allow "maniphest.subtypes" to configure which options are presented by "Create…
D19853 / rPd1bcdaeda467: Allow the "Create Subtask" workflow to prompt for a subtype selection, and…
D19857 / rPbf6c534b567a: Give "Track Only" repository detail proper getters/setters
D19856 / rPc3206476a303: Give "Autoclose Only" repository detail proper getters/setters
D19850 / rP1a6a0181a898: Allow "bin/repository thaw --demote" to demote an entire service, not just a…
D19849 / rPbba418600591: Allow "bin/repository thaw" to accept "--all-repositories" instead of a list of…
D19851 / rP1e4bdc39a11b: Add an "availaiblity" attachment for user.search
D19852 / rPf0eefdd0b58b: Replace the informal "array" subtype map with a more formal "SubtypeMap" object
D19848 / rP5d54f26daca8: Support reading and querying Almanac service PHIDs via "diffusion.repository.
D19841 / rP01c7be059dd6: Add support for "harbormaster.target.search"
D19842 / rP2f11001f6e3c: Allow "Change Subtype" to be selected from the comment action stack
D19843 / rP1d0b99e1f834: Allow applications to require a High Security token without doing a session…

Related Objects

Mentioned In
T7667: Provide `auth lock` and `auth unlock` to restrict authentication provider management to the CLI
T13138: Improve consistency of MFA requirements to invite/approve users
T13134: Give Conduit API tokens PHIDs, transactions, and other modern capabilities
T13229: On Third-Party Integrations
T13225: Complete session digest migration from SHA1 to SHA256
Mentioned Here
T13242: 2019 Week 5 Bonus Content
D19975: Convert user MFA factors to point at configurable "MFA Providers", not raw "MFA Factors"
T13227: Figure out if Google Auth needs to be updated before Google+ shutdown on March 7, 2019
D19968: Fix an issue where transactions in mail were always rendered as text
T13115: Handle mail bounces inside Phabricator
D19935: Add a skeleton for configurable MFA provider types
T920: Provide SMS Support
T7477: Handle inbound email with phabricator address in the CC
T12921: Link to referenced object in transaction emails
T13229: On Third-Party Integrations
T8787: Add support for U2F MFA once browser implementations improve and compatible hardware is more widely available
D19914: Make Images in Pholio refer to mocks by PHID instead of ID
T13226: Consider login/session alerts, and other security alerts (for example, around MFA)
rP1d0b99e1f834: Allow applications to require a High Security token without doing a session…
T7667: Provide `auth lock` and `auth unlock` to restrict authentication provider management to the CLI
D19856: Give "Autoclose Only" repository detail proper getters/setters
D19857: Give "Track Only" repository detail proper getters/setters
T10743: Test notifications should not be broadcast globally in feed
D19829: Modularize Repository transactions
T12588: create subtask side menu to allow creation of subtasks of different types to the parent task
D19792: Use 160-bit TOTP keys rather than 80-bit TOTP keys
T5055: Distribution mechanism for arc extensions
T6115: Allow multi-factor authentication to be a requirement for user subgroups, including administrators
T6703: Allow multiple copies of the same auth provider type
T9770: It is possible to use the same 2FA token more than once
T13044: SAML Support
T13138: Improve consistency of MFA requirements to invite/approve users
T13156: Plans: Improve Phacility UI for managing instance managers and cards

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Also, just so I don't forget:

  • Edit forms should get "workflow" if we're going to prompt you when you submit them.
  • Also, comment interactions should get a warning when you'll be required to MFA on submit, especially when you don't have MFA on your account.

I expect to make TOTP configuration a concrete object, then pursue Yubikey/U2F (T8787) and Duo (PHI912).

  • U2F seems premature today given uneven browser support (see T8787 for more detailed discussion).
  • Duo seems like it's actually a very good technical solution today, it's just unfortunate that it's a Cisco "Contact Sales" outfit now. This will entangle with T13229 somehow.

I maaaaaybe want to take a look at SMS just to figure out where it's at, too, although I think there's a mountain of evidence at this point that SMS MFA is very weak. I'm more interested in SMS for push notifications than MFA.

Given that U2F seems premature, I'm slightly more interested in SMS to get a second MFA integration upstream to better prove that the implementation is general enough to support push-style MFA factor types. Still, not sure where this is going to go.

D19935 prepares for multiple MFA provider types.

Before implementing an external Duo provider, I'd like to implement at least a second MFA provider in the upstream so Duo is less of a one-off. SMS (T920) seems like the most obvious choice since it's generally similar to Duo (both are API-based push factors).

Modern SMS implementation triggers some refactoring in mailers, and mailers also have some legacy code on the chopping block after the cluster.mailers change about a year ago. Some configuration simplification here is generally triggering nearby issues like T7477, and some backlogged issues like T12921 probably make sense to resolve here because of testing overlap.

(I'm just going to stick with this "Bonus Content" until we wrap MFA, however long that ends up taking, since there's a bunch of errata collected here.)


One of the changes here involves removing the default "To:" placeholder and replacing it with a voided placeholder instead. Adjacently, we should probably prevent applications from claiming RFC2142 email addresses, including abuse@ and hostmaster@. See also https://hackerone.com/reports/397792. Some addresses like support@ and security@ are fine, but addresses which might conceivably be used by CAs to validate domain ownership (ssladmin@) or MTAs / mailing lists (majordomo@) to do whatever inscrutable things MTAs do should likely be reserved and rejected.

One issue here is that existing MFA factors now require a concrete Factor object to associate with. To preserve existing behavior, we'd need to create a new one automatically on all installs. This inevitably turns into a little bit of a mess where we have some builtinKey factors and some organic factors. Nothing terrible, but not especially clean.

However, since "not supporting MFA" is a legitimate configuration state, I think a cleaner approach is probably to just migrate:

  • If no user has TOTP MFA, bail.
  • If no TOTP provider exists, create one. Otherwise, pick the first one.
  • Point unattached TOTP factors (NULL value for factorProviderPHID) at whichever one we created/picked.
  • We're done forever.
  • Document how to do MFA config.

This is significantly simpler. The cost is that to enable TOTP MFA for the first time on a new install, you now need to go click a button somewhere, but that's probably good as we move forward toward broader MFA support since "just use TOTP" is no longer a completely unambiguous choice.

since there's a bunch of errata collected here

Here's a rollup:

One small leftover from D19792: it would be nice to show the bit-strength of TOTP secrets in the UI.

Tangential thought: at some point, the "Has MFA" filters in People should perhaps move to somewhere in Auth, with a more detailed UI for reviewing and managing MFA across user accounts.

Finally, T13138 notes a couple of MFA workflow/consistency improvements which could be revisited.

One adjacent change is T7667 (bin/auth lock / unlock).

Another change I'd like to make is to restore the "login page instructions" configuration. This was once a insecure blob of arbitrary HTML which we rightly got rid of, but we could let you put a blob of remarkup there easily enough. For secure, that would let us clarify that registration on this install is not open.

From rP1d0b99e1f8348ca222498551423154a0c37878fb, I'd like to add a hard-cap on password attempts per time period per remote address regardless of whether CAPTCHAs are configured or not. If you can crack CAPTCHAs or an install doesn't configure them (very reasonable given that configuring them lets Google run arbitrary Javascript on your install), we currently fail fairly wide open.

Existing flows should get an MFA workflow key where practical.

Where it makes sense, "High Security" workflows (that keep you in "sudo" mode) should also be reduced to "MFA" (one-shot) workflows. A good example of a workflow which can reasonably be reduced like this is the "Rename User" workflow. Another good examples is the Passphrase "Reveal Credential" workflow.

Edit forms should get "workflow" if we're going to prompt you when you submit them.

Also, comment interactions should get a warning when you'll be required to MFA on submit, especially when you don't have MFA on your account.

When no providers are configured, the MFA panel should probably vanish.

The new MFA stuff should be documented.

Statuses for providers ("Disabled", "Deprecated") don't do anything yet, but you can't edit them anyway.

And the remaining non-errata stuff is:

  • SMS MFA.
  • Duo MFA.

...which hopefully we're now in striking distance of.

epriestley added a comment.EditedJan 16 2019, 3:51 PM

SMS MFA

There are two pieces here before the actual MFA piece:

  1. Sending SMS via MetaMTAMail.
  2. Storing phone numbers for users.

(1) is likely straightforward.

For (2), one option is just to store the phone number on the factor: you click New MFASMS and enter a phone number. However, I think we're likely to outgrow this almost immediately given the other cases for SMS in T920, the likely need to verify/block/track-blocklist-state of numbers eventually (T13115), the difficulty of updating phone numbers, desire to prevent duplicate phone numbers, and so on.

So I'm leaning toward SettingsContact Numbers or similar. For now this is probably just basic phone without too many fancy features, and New MFASMS says "Add a phone contact number first.". This could grow into APNS/Whatsapp/GCM/whatever eventually, and the pathway is pretty straightforward (no complicated migrations, etc).

Duo MFA

When we get there, I'm just going to put this one on the factor. Alternative is to make it an auth provider, but I think "login with Duo" isn't really much of a thing and since I'm not planning to bring the actual Duo magic sauce upstream I don't want to complicate things too much.

The fix for T12921: Link to referenced object in transaction emails (in D19968) is a bit buggy -- the links aren't generating as absolute links with a protocol and domain name. This is likely an easy fix, although I don't expect to get to it tonight.

I'm also not in love with linking "epriestley did stuff..." in every transaction. That seems pretty low value, especially when the actor is an application. I'm inclined to see if I can find an easy way to squelch that.

T13227 also needs to happen this week.

epriestley added a comment.EditedJan 21 2019, 5:26 PM

From D19975, two considerations:

  • The "MFA Required" flow (login to a new account with no MFA, when require-mfa is configured) needs to be carefully tested to make sure it works properly, particularly when only one SMS provider is enabled (users must be able to set contact numbers). This may motivate reworking the "MFA Required" screen into more of a "subset of Settings" screen.
  • We should have better documentation and/or tooling around session resets after MFA changes. Today, users who have existing MFA configurations will not be automatically required to update them after MFA provider changes (their existing sessions remain valid). You can bin/auth revoke --type session to force this, but we should provide clearer guidance around it and/or a "downgrade sessions to partial" tool.

The need for more UI support on the "MFA Required" flow is making me consider expanding scope here into knocking "Settings" back out to a full-width UI. I think that's not terribly hard. I'm maybe going to start there so it can get more testing as I go through everything else, and I can give up if it's harder than I think.

So I don't forget about it: the default settings/ UI is now incorrectly showing Conpherence settings, not date and time settings. Clicking anything works properly.

Also, I think we still have an issue with rate limiting where:

  • You type some stuff in.
  • The challenge has expired, and you get hit by "Wait Patiently".
  • You still get points against you for rate limiting.

Currently, I think attackers can CSRF the MFA enroll endpoint (/mfa/new/?provider=xyz). This could send you an SMS, soon. This is mostly harmless but potentially a little annoying with push factors like SMS/Duo. I'll get a CSRF gate on it.

Also, managing contact numbers needs to require MFA or evading SMS MFA is trivial. 🐈

managing contact numbers

This also means that we need to prevent you from disabling or editing your primary contact number if you have SMS MFA configured: "You have SMS MFA configured, so you can not change your primary contact number. Remove the SMS MFA factor to edit your contact number."

This is a bit roundabout, but is actually the right flow with "MFA Required" I think -- it temporarily puts you back in MFA jail until you sort things out.

This will probably roll forward into a "next iteration", but it would be nice-to-have to show users the numbers we expect to text them from, so they can whitelist/anticipate the messages.

This is straightforward with Twilio and presumably also reasonable with SNS, although it might get a little tricky if an install has 9 providers configured in some complex series of fallbacks.

Rolling this mess up again:

  • T13227 needs to happen.
  • The actual SMS prompt needs CSRF.
  • Read-Only / Disable for providers should be editable.
  • MFA settings panel state for no read-only / no providers.
  • Guidance on "Required" gate for read-only / no providers.
  • "Kick everyone down to partial session" docs / tooling.
  • MFAEngine usability:
    • When MFAEngine will require MFA, warn users before submitting comments.
    • When MFAEngine will require MFA, warn users before submitting EditEngine forms and put workflow on them.
  • Existing Flows:
    • Reduce "require MFA session" to "require MFA token" where possible.
    • Put workflow keys on existing flows.
    • T13138 notes flow inconsistency on existing checkpoints.
  • bin/auth lock/unlock
  • Maybe move "who has MFA?" tools from the user directory to Auth.
  • Show TOTP secret bit strength.
  • Documentation.
  • Some other stuff on the Auth workboard although this is mostly splash damage.
  • Maybe guidance to users about "from" numbers, although this looks nontrivial for SNS.
  • Maybe customizable enroll guidance.

I think the only non-cleanup sort of thing left is that the Duo provider needs to have editable fields, while the upstream (TOTP, MFA) providers do not. This probably means Duo integration ends up being three files (factor itself, plus two transaction types for "credential" and "hostname"), but that seems alright, and could be cheated through with a generic transaction implementation for the moment.

"Kick everyone down to partial session" docs / tooling.

This turned out to be mostly trivial since PhabricatorUser has a cache we can safely invalidate without too much effort.

epriestley closed this task as Resolved.Sat, Jan 26, 3:00 PM

Continued in T13242.