Page MenuHomePhabricator

2018 Week 48-51 Bonus Content
Open, NormalPublic

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

Differential Revisions
D19898: Tighten some MFA/TOTP parameters to improve resistance to brute force attacks
D19897: Allow any transaction group to be signed with a one-shot "Sign With MFA" action
D19896: In Legalpad, prompt for MFA at the end of the workflow instead of the beginning
D19895: Carry MFA responses which have been "answered" but not "completed" through the MFA workflow
D19894: Explicitly mark MFA challenges as "answered" and "completed"
D19893: When accepting a TOTP response, require it respond explicitly to a specific challenge
D19890: Simplify and correct some challenge TTL lockout code
D19889: Bind MFA challenges to particular workflows, like signing a specific Legalpad document
Commits
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…

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
epriestley updated the task description. (Show Details)Mon, Nov 26, 4:15 PM

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

This is really two support issues (PHI873 and PHI912) and overlaps slightly with T13156 and touches T9770.

PHI873 has a lot of very narrow use cases, but the features I'd like to build are:

One-Shot MFA: Currently, when we hit you with an MFA check we always leave your account in high-security mode afterwards (except on login). This is appropriate sometimes (e.g., when managing email addresses, it's common to need to make several changes in a row) but not necessarily appropriate at other times (when signing a Legalpad document, you rarely need to stay in MFA). I'd like to add a one-shot MFA mode and change existing callsites to one-shot where it's the more appropriate mode.

MFA Triggers: PHI873 would like a custom "provide MFA signature" sort of action. I'm going to write this as custom code for now since it's not clear how much upstream value there is, but this is something we could consider bringing upstream.

Herald fires too late to trigger this, so we can't write a [Require MFA Signature] action in Herald. We could attach "Require MFA" to something like maniphest.statuses, but this feels very fiddly. We also have no way to handle MFA over Conduit today.

PHI912 discusses a few things but basically boils down to "implement Duo".

Duo: I'm planning to implement this as an authentication provider and an MFA provider if it doesn't prove technically bankrupt (see T13044 for SAML, previously). I'm chilly on upstreaming this since Duo is basically a giant "Contact Cisco Sales" organization at $9/user/month. It feels like we get burned pretty much every time we do the legwork to integrate with a third party paid services like this and I'd like to find better ways to make these integrations feel valuable to us. A possibility is that we provide these things as per-user licensed extensions after T5055 and have a policy that integrations with third-party paid services are also paid.

The overlap between T13156 and this is:

Disable MFA Completely: Today, there's no way to actually prevent users from adding MFA, but I'd like to completely disable MFA on admin.phacility.com. The ideal solution is probably more nuanced, like "allow users to add MFA to their Phacility accounts if they promise they know what they're doing" but that's probably out of reach. Disabling MFA completely is an acceptable replacement. The goal here is to reduce support burden around users configuring both central (admin) and instance MFA and being super confused about which is which.

Currently, MFA can only be made universally required or universally optional with security.require-multi-factor-auth. T6115 would like support for rules like "Administrators must have MFA". We could imagine a more complete implementation might break users into several buckets that apply different MFA requirements:

[ Administrators ] require [ Good MFA ]
[ Employees ] require [ Good MFA or SMS ]

At least for now, this feels like a big mess so I'm not planning to touch it. Because configuration here is probably around tiers of access/security, I think it's somewhat orthogonal to provider configuration: you aren't likely to want to configure "SMS MFA" as "required for employee group X", but rather "employee group X" as "requires SMS or better".

For now, provider configuration probably looks like giving each provider an "Enabled | No New Registration | Disabled" state and letting you fully disable MFA by disabling all providers. The "No New Registration" state would continue enforcing existing MFA tokens but not allow users to add new ones (so you could sunset an older provider). Then we can put TOTP in sunset mode on admin.phacility.com.

This should learn from Auth and support multiple providers of the same type from initial implementation (see T6703). Multiple TOTP providers aren't distinct today but future providers may be, and the cost is small now but not small later.

For T9770:

Bind Challenges to Sessions: See also https://hackerone.com/reports/435651. I'd like to defuse the "spyglass" attack primarily by binding challenges to sessions. I believe this is broadly more effective than preventing token reuse.

We could also prevent token reuse, but I'd like to make sure one-shot interactive prompts save a successful response across submissions if we pursue this. If you go to sign a Legalpad document, get prompted, and then the form doesn't actually go through (typo / missing field / whatever) you shouldn't be locked out for 45 seconds.

This probably looks like a <userPHID, challengeWindow, sessionID, responseToken?, details> sort of table. Initially, we insert this with a NULL responseToken, which means "the user has requested a challenge for the given window". When they respond correctly, we generate a random secret and store the digest in responseToken. The secret goes back to the client as a hidden form input and lets them pass the same challenge without re-entering stuff into the form over and over again, but the challenge is otherwise no longer respond-able. challengeWindow probably needs to be more abstract since, e.g., SMS won't have a time-oriented window. So maybe this is really challengeKey (timestep for TOTP, message ID for SMS, etc) plus MFA ID, and the client token becomes a list of challenges you've responded to. That seems like it covers all the bases.

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.

epriestley updated the task description. (Show Details)Mon, Nov 26, 11:13 PM
epriestley updated the task description. (Show Details)Mon, Nov 26, 11:25 PM
epriestley updated the task description. (Show Details)
epriestley updated the task description. (Show Details)Tue, Nov 27, 12:46 PM
epriestley updated the task description. (Show Details)Wed, Nov 28, 10:04 PM
epriestley updated the task description. (Show Details)Thu, Nov 29, 6:41 PM
epriestley renamed this task from 2018 Week 48 Bonus Content to 2018 Week 48-49 Bonus Content.Mon, Dec 3, 5:48 PM
epriestley updated the task description. (Show Details)
epriestley updated the task description. (Show Details)Mon, Dec 3, 6:07 PM
epriestley updated the task description. (Show Details)Wed, Dec 5, 8:36 PM
epriestley updated the task description. (Show Details)Wed, Dec 5, 9:32 PM
epriestley renamed this task from 2018 Week 48-49 Bonus Content to 2018 Week 48-50 Bonus Content.Mon, Dec 10, 2:20 PM
epriestley updated the task description. (Show Details)
epriestley updated the task description. (Show Details)

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').

I didn't fix all of these, but D19856 and D19857 are the two most egregious/complex ones, and a number of the others will probably get changed sooner or later anyway.

epriestley updated the task description. (Show Details)Mon, Dec 10, 4:09 PM
epriestley updated the task description. (Show Details)Mon, Dec 10, 7:09 PM
epriestley updated the task description. (Show Details)Mon, Dec 10, 11:24 PM
epriestley updated the task description. (Show Details)Tue, Dec 11, 12:28 AM
epriestley updated the task description. (Show Details)Wed, Dec 12, 7:30 PM

This should learn from Auth and support multiple providers of the same type from initial implementation (see T6703).

I'd also like to consider fixing this as part of the same scope. There's likely nothing particularly technically challenging about it, and giving both datasets the same general structure should improve consistency.

The value of fixing T6703 is very low, but it's architecturally clearly "wrong", I'd like to fix it, and it seems unlikely to ever get much more valuable.

One adjacent change is T7667.

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.

I'm not entirely sold on expanding the scope here, but maybe...

Bind Challenges to Sessions

I think I've written most of what I discussed above locally, but sequencing it is a little tricky. Just thinking out loud, my plan here is:

  • Mostly just for consistency, give sessions real PHIDs.
  • Add a Challenge table to bind challenges to sessions. This will implement these cases (which are the same thing for TOTP, but the implementation should support them being different things for other token types):
    • "You can not provide a TOTP token because another session has already requested a TOTP token for this timestep. [Wait Patiently]"
    • "You can not provide a TOTP token right now, wait X seconds. [Wait Patiently]"
  • Add a workflowKey to lock challenges to workflows. This will implement this case:
    • "You can not provide a TOTP token here because you've already asked for one for this timestep on some other workflow. [Wait Patiently]"
  • Add a responseToken to track responses. This will implement this case, and prevent all token reuse:
    • "You can not provide a TOTP token here because you already answered a TOTP challenge for this timestep. [Wait Patiently]"
  • Allow responseToken values to persist within the same workflow, enabling token reuse for the same user in the same session on the same workflow for a short period of time, so we tell users to "Wait Patiently" less frequently.
  • Move transactional MFA prompting inside the TransactionEditor so transactions validate before MFA prompts, further reducing "Wait Patiently".
  • Implement a "sign with MFA" transaction to force a prompt. (Does this come upstream?)
  • Look at how much adjacent Auth stuff is going to happen in this scope.

One piece of minor mess here -- when you bin/auth recover yourself into a MFA'd account, you can get two MFA prompts: one to upgrade the session, then one to allow you to perform a password reset. Probably, the contextless password reset should only require MFA if you actually submit the form, and should do one-shot MFA, and ideally should carry the challenge tokens from the login and belong to the same workflow, although that's probably impractical.

epriestley renamed this task from 2018 Week 48-50 Bonus Content to 2018 Week 48-51 Bonus Content.Mon, Dec 17, 2:51 PM