Page MenuHomePhabricator

Short term plan for security and private tasks
Closed, ResolvedPublic

Description

We need a short term plan for private tasks fully agreed by @csteipp and the Phabricator team. Currently we have a system that works and several open issues. Before attempting to solve the issues one by one, we need to be on the same page with the overall plan.

Steps:

  1. Collect all the affected tasks as blocked tasks.
  2. Write down a description of the current implementation (done) and the aspects that need to be improved.
  3. Sit down, discuss and edit until we have a common plan.

Then we will proceed resolving the tasks accordingly.

FeatureImplementationExpectationHappy with current implementation
Making a task privateSecurity dropdown sets access control template via HeraldVisibility status independent of projectsAs an interim solution yes. As a definitive solution, maybe not. Upstream plans to work on Spaces.
Associating projects to private tasksYesYesYes
T475: Authors of private tasks should be able to access them YesYesYes
Access for authors of Bugzilla migrated private tasksOnly after the task is updatedYesYes, by now most issues are updated, and if there is any remaining updating it is easy.
T518: Users CCed in private tasks should be able to access themExternal users CCed receive notifications but they cannot view or edit the tasks. Any exceptions need to be handled out of the Security template.YesNo, and this is the biggest problem currently. Users CCed must be able to view and edit. Changing the policy manually is causing extra work for @csteipp. It's considered a regression from the situation we had in Bugzilla.
CCed users can add other CCed usersNo, by design.Yes, as we could in BugzillaNo, although this is not as urgent/important as CCed users not being able to view/edit.
Access for CCed users in Bugzilla migrated tasksProbably the same as above after the task is updated.Same as aboveNo, same as above
Files uploaded directly to a private task inherit private policyYesYesYes, no problems found so far.
Thumbnails of private images should be privateNo, it would take a big performance hit and with such small size doesn't disclose anything. You need to know the exact URL of the thumbnail. Upstream agrees.YesNo, this is a requirement we also have for MediaWiki, and we are paying the performance penalty there as well. @csteipp is happy to put us in touch with the colleagues that fixed this in MediaWiki. Not urgent, but it needs to be a goal in our plans.

Event Timeline

Qgil created this task.Dec 1 2014, 11:09 PM
Qgil claimed this task.
Qgil raised the priority of this task from to High.
Qgil updated the task description. (Show Details)
Qgil changed Security from none to None.
Qgil added a subscriber: Qgil.

I think I have added all the relevant tasks as blockers. If I missed any, please add it.

Qgil updated the task description. (Show Details)Dec 2 2014, 2:17 PM

I'm still polishing details and reviewing all the comments in the tasks related. Is there any feature missing? Any detail misunderstood?

Qgil updated the task description. (Show Details)Dec 2 2014, 2:49 PM

Users in the security group should be able to add external users. External users CCed not being able to add other external users is fine (right?)

No. Like with bugzilla, cc'ed users should be able to edit and add more users.

Security extension strips any users CCed manually?

I'm pretty sure this is incorrect. The policy / extension / enforcement / whatever-they're-called strips anyone except the original reported who was added to the "Visible by" and "Editable by" policies. CC's are left in tact, leading to the further undesirable issue that those users are CC'ed when the bug changes, but they are not in the ACL, so they can't view the actual bug.

Thumbnails of private images should be private

...

Maybe yes?

No, not happy with the current situation. We accept the performance hit of checking permissions when accessing images (including thumbnails) on office wiki. For the number of users we're dealing with in Phab, I'm sure we can make it work.

JanZerebecki updated the task description. (Show Details)Dec 2 2014, 5:52 PM

For the number of users we're dealing with in Phab, I'm sure we can make it work.

This is possible without taking the performance hit on public files by checking at page output time (you need to already do that for the task itself) (and at file upload time and permission change time). Then deliver non-public files/images from a URL that checks permission and the public image from a different URL that does not check. That way it even works when a CDN is in use. Only non-public files then either don't use the CDN or use the access control or access token feature of the CDN.

Qgil updated the task description. (Show Details)Dec 2 2014, 8:35 PM
Qgil updated the task description. (Show Details)Dec 2 2014, 8:43 PM
Qgil updated the task description. (Show Details)Dec 2 2014, 8:48 PM
Krenair added a subscriber: Krenair.Dec 2 2014, 9:53 PM

@JanZerebecki & @csteipp: Although you are right, it is possible to differentiate at page output to differentiate between private and public files, it's not really possible without some fairly complicated changes to phabricator code.

There are two issues with this:

  1. Development time/budget: Obviously this would take some time and my time is about to go back to release engineering, which is the job I was hired for and have been neglecting so far due to phabricator tasks taking up 100% of my time.
  2. Ongoing maintenance / upstream acceptance: Although I believe the upstream phabricator project would accept a patch from us to improve the security of uploaded files, the implementation would have to be something that @epriestley is happy with and fits within the phabricator architecture. If we hack something together that works for us but isn't architecturally correct then we will be stuck maintaining it as a fork of phabricator. Any forked code that we take on will turn into long-term technical debt that we are trying to avoid as much as possible.
Qgil added a comment.Dec 2 2014, 9:56 PM

Please, let's keep this task focused on "Short term plan for security and private tasks". The thumbnailing problem should be discussed in {T75774}.

Krenair updated the task description. (Show Details)Dec 2 2014, 9:58 PM

Does anyone have a plausible scenario in which a 280x210 thumbnail of an image (which is the largest we'll generate) which requires a long, secret key to access is the failure that allows an undesired disclosure?

Particularly, @JanZerebecki seems to imply (above) that it's OK if a public URI exists, just not OK for it to be used on the task.

It's not particularly difficult to take the performance hit on thumbnails, but I can't come up with a plausible scenario where this matters, and it's slightly simpler to treat all thumbnails uniformly.

Reminder when you write stuff that's more than just a link: This is a public task.

Regarding {T75774}, I have pasted the full list of items there and I prefer to leave judging whether one creates a problem to Chris. Let's please discuss T75774 in T75774 if possible (though T518 might make that harder). Thanks for your understanding.

Qgil added a comment.Dec 3 2014, 7:20 AM

Today we have Wikimedia Phabricator team meeting. We will go through the table of features above with the goal of being on the same page the four of us. Then we will schedule a meeting with @csteipp (and whoever else is needed) for next week with the goal of agreeing on the plan.

Qgil moved this task from Backlog to Doing on the ECT-December-2014 board.Dec 3 2014, 8:55 AM
Qgil added a comment.Dec 4 2014, 8:18 AM

@chasemp wrote the very useful https://www.mediawiki.org/wiki/Phabricator/Security document. I'm going to update the related tasks accordingly and I ill update the bale above.

All in all it looks like we have broad agreement and a short term solution for the CC users problem. More to be discussed in T518.

Currently there is no agreement on the thumbnails of private files. After our Wikimedia Phabricator meeting yesterday, the team has now a commo opinion that I will discuss with @csteipp and whoever else needs to be in the loop. Either we all agree that the current situation is good (our preferred option, backed by the upstream plan and industry practices), or we need to plan and resource a patch that might or might not be taken upstream.

The rest seems to be all good, or just minor details that can be addressed with normal priority.

Qgil updated the task description. (Show Details)Dec 4 2014, 8:20 AM
Qgil moved this task from To Triage to Doing on the Phabricator board.Dec 4 2014, 1:26 PM
Qgil lowered the priority of this task from High to Normal.Dec 10 2014, 11:41 AM

Setting priority to Normal. There are still a few details under discussion preventing us to resolve this task, but the overall plan is clear and nobody is blocked here.

Qgil closed this task as Resolved.Dec 31 2014, 9:38 AM

Well, I think the short term plan is clear right now. There are some details still being discussed in some of the Blocked By tasks, but there is nothing that should stop @mmodell and others working on the improvement of our current security features. Closing as Resolved.

Restricted Application added a subscriber: scfc. · View Herald TranscriptJun 20 2015, 5:02 AM