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.

Revisions and Commits

Restricted Differential Revision
rPHU libphutil
D19981
D19882
rP Phabricator
D20038
D20037
D20036
D20035
D20034
D20033
D20032
D20031
D20028
D20024
D20023
D20022
D20021
D20020
D20019
D20018
D20008
D19976
D19975
D19994
D19992
D19997
D19987
D19965
D19953
D19935
D19943
D19909
D19908
D19906
D19905
D19904
D19903
D19902
D19901
D19900
D19899
D19898
D19897
D19896
D19895
D19894
D19893
D19890
D19889
D19888
D19886
D19885
D19884
D19883
D19881
D19867
D19866
D19865
D19864
D19861
D19860
D19859
D19858
D19855
D19854
D19853
D19857
D19856
D19850
D19849
D19851
D19852
AuditedD19848
D19841
D19842
D19843

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.Jan 26 2019, 3:00 PM

Continued in T13242.