csteipp wrote on 2014-04-21 16:01:48 (UTC)

We can split this into Ops and MediaWiki specific pieces if we need to, but I'll document the MediaWiki side, since the original task was about ops.

In bugzilla, we have a Security product. Bugs in that product can only be seen by people in the security group.

• By default, the reporter and anyone CC'ed can also see the bug, which is important since,
• The reporter will want to track the status of the bug
• We often have non-security people working on the bug, and they need visibility to the discussion, but we wouldn't want to give them access to all security bugs.

• Bugs are moved out of security and made publicly accessible once the issue is resolved. This is important for historical tracking and institutional learning. We could duplicate them, but that would be a pain.

• Security bugs are sometimes opened in public categories, and then moved to Security once someone realizes it has security implications. Since these usually have gone out to IRC and via email, we assume they are mostly public, but it's nice to protect further conversation on the bugs until the issue is resolved.

• A handful of times each year, someone reports private data as part of a security bug (IP addresses, etc). We need to be able to make those specific comments invisible, or be able to delete them.

qgil wrote on 2014-04-22 21:50:26 (UTC)

Thank you @csteipp, I have created a meta-task upstream.

T4868: The case of the Security project

qgil wrote on 2014-04-24 05:36:43 (UTC)

I have ended up creating a task mapping directly this one: https://secure.phabricator.com/T4893

This is one of the main bottlenecks we have found for a potential migration from Bugzilla / RT.

qgil wrote on 2014-04-25 04:53:05 (UTC)

◀ Merged tasks: T96.

qgil wrote on 2014-04-25 04:56:21 (UTC)

Merged T96: Dependency tree view of tasks. The theoretical workflow would be simple: create a task, add it to the project with private policy, done. As author of the report you should be included, as defiuned by @csteipp above.

aklapper wrote on 2014-04-25 08:07:01 (UTC)

As I wrote in https://secure.phabricator.com/T4893 :
Phabricator allows a task to be part of more than one project. A user can create a ticket with 'Projects' set to 'Foobar' (as that's the codebase) and also 'Security' (as it's a vulnerability s/he reports). We'd want to make sure that the access settings for 'Foobar' wouldn't "leak" the information in the ticket and that the access rules of the "more restrictive" project are applied to the task, I guess.

qgil wrote on 2014-05-01 06:13:49 (UTC)

Moving this task to "Not critical for the RfC" because there is little else here to discuss. This is a feature we need, and we have documented this at https://www.mediawiki.org/wiki/Requests_for_comment/Phabricator/versus_Bugzilla#Missing_features

qgil wrote on 2014-05-13 19:03:55 (UTC)

As agreed today, @mmodell will look into this critical task. The idea is to sync upstream and start contributing patches.

aklapper wrote on 2014-05-19 13:16:30 (UTC)

Related holes that I see to fill:

• If a task is in more than one project and one of the projects has restricted access permissions defined, the access permissions of the most restrictive project must be applied to that task (e.g. adding the public project 'Foo' to a task in the restricted project 'Secret' should likely not send mail notifications to people watching 'Foo' in Herald).
• Obviously, after user XY has set up Herald to receive all mail notifications for actions of the Security Engineer, Phab should check above task access restrictions and Phab should only send mail notifications for actions of the Security Engineer on those tasks which are also accessible to user XY. (This is how Bugzilla works.)
• If dependencies between a public task (T99999) and a restricted task (T100000) exist, the public task T99999 should preferably display "depends on: T100000" but should NOT expose the task title ("depends on: T100000: foo_bar() allows XSS/CSFR" or whatever) in the UI. Same for "removed dependencies:" and "added dependencies:" in mail notifications.

qgil wrote on 2014-05-19 22:23:06 (UTC)

The work on "Spaces" has started upstream: https://secure.phabricator.com/D9204

aklapper wrote on 2014-06-13 15:43:06 (UTC)

@mmodell: Had a chat with @Rush, potentially changing https://www.mediawiki.org/wiki/Phabricator/Plan#Migration_plan so we ended up wondering how far you are here. Comments appreciated:

<chasemp> we can't enable SUL before we finish the security access features. as we can do legalpad + SUL or security features + rt + SUL + legalpad but we can't do rt + legalpad + SUL without security features. basicaly, SUL and security features have to land at the same time. so that puts the security features in the path of SUL?
<andre> naive question: why would starting with legalpad + SUL, and later applying security features + importing rt tickets to test not work?
<chasemp> it would but that means the moment we enable SUL we can't test any import until security features lands
<chasemp> which defeats the original purpose of doing any of this at all
<andre> so probably a good followup question is how far Mukunda is with security features (T95; "Spaces" app in upstream)
<chasemp> agreed, that's next Q
<andre> because that would influence the order of steps

Rush wrote on 2014-06-13 15:53:18 (UTC)

Thanks andre, yeah I was going through my notes to respond to the updated / suggested schedule in another ticket. I'm sorry I didn't have this on brain yesterday but essentially, this ticket has to land with SUL in order to work with importing either rt or bugzilla tickets (I think bugs has privately reported bugs?).

Hopefully this can be wrapped up in a few weeks time?

mattflaschen wrote on 2014-06-13 19:16:19 (UTC)

In T95#35, @Rush wrote:

Thanks andre, yeah I was going through my notes to respond to the updated / suggested schedule in another ticket. I'm sorry I didn't have this on brain yesterday but essentially, this ticket has to land with SUL in order to work with importing either rt or bugzilla tickets (I think bugs has privately reported bugs?).

Yes, I believe the only private bugs in Bugzilla are security bugs, but that's obviously an important "only". See @csteipp's comment above above for details.

aklapper wrote on 2014-06-13 19:53:35 (UTC)

The only Bugzilla tickets who have access restricted to a certain group (members of the Bugzilla group security) are bug reports under the Bugzilla product Security AFAIK, but some attachments and some comments (spam or exposing private information) are also marked as private and only visible to members of the group admin which is set as the insidergroup in Bugzilla's settings.
Writing this just for the records, as this tickets is about project-level specific access restrictions.

qgil wrote on 2014-06-13 20:08:56 (UTC)

In T95#37, @Aklapper wrote:

some attachments and some comments (spam or exposing private information) are also marked as private and only visible to members of the group admin which is set as the insidergroup in Bugzilla's settings.

But these wouldn't be related to the "Security" project in Phabricator. These should be deleted as Phabricator understand this term by default: present in the database but hidden to anybody but admins with access to the database (right?).

Rush wrote on 2014-06-17 16:42:28 (UTC)

we are going to be bumping up against this sooner than later. Any idea of ETA?

Rush wrote on 2014-06-17 17:11:14 (UTC)

Thought of another thing, didn't see any mention in upstream. File attachments need to preserve the hidden quality of their projects, is this on the table currently?

aklapper wrote on 2014-06-17 17:50:46 (UTC)

Also see https://secure.phabricator.com/T4589 for file access restrictions (pointed out by Rush) - in the current WMF setup, patches for security issues are often attached in Bugzilla (instead of uploading to Gerrit where they would be visible).

mmodell wrote on 2014-06-17 18:11:33 (UTC)

Wouldn't it be good enough that the task containing the attachment is locked and therefore the file attachment wouldn't be discoverable without access to the task?

Rush wrote on 2014-06-17 18:20:18 (UTC)

http://fab.wmflabs.org/P8

csteipp wrote on 2014-06-17 19:59:02 (UTC)

In T95#42, @mmodell wrote:

Wouldn't it be good enough that the task containing the attachment is locked and therefore the file attachment wouldn't be discoverable without access to the task?

For security bugs, I would prefer that even if the url is known, unauthorized users can't get access (how bugzilla does it). Beyond security patches, we sometimes have private (deleted/suppressed) data reported in bugs.

mmodell wrote on 2014-06-17 20:05:19 (UTC)

This is the code that epriestley pointed to as an example of how to create and attach a file with nobody-can-read policy:

private function loadFileForData($path, $data) {
  $file = PhabricatorFile::buildFromFileDataOrHash(
  $data,
  array(
  'name' => basename($path),
  'ttl' => time() + 60 * 60 * 24,
  'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
  ));

  $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
  $file->attachToObject(
   $this->getRequest()->getUser(),
   $this->getDiffusionRequest()->getRepository()->getPHID()
  );
  unset($unguarded);

  return $file;
 }

We could create a simple CLI php script that would import the files in a manner similar to this and then we just need to attach them to the tasks in maniphest.

This is either dependant on or could benefit from the following upstream tasks:

https://secure.phabricator.com/T4589 and https://secure.phabricator.com/T4896

mattflaschen wrote on 2014-06-17 21:51:01 (UTC)

My understanding is https://secure.phabricator.com/T4589 is a blocker for the file part

In T95#44, @csteipp wrote:

For security bugs, I would prefer that even if the url is known, unauthorized users can't get access (how bugzilla does it). Beyond security patches, we sometimes have private (deleted/suppressed) data reported in bugs.

Especially since the file URLs just increase in numeric order, so it's trivial to enumerate recent ones (even if there's a rate limit).

aklapper wrote on 2014-07-02 16:28:04 (UTC)

@mmodell: I'm slightly afraid this ticket might become the bottleneck in about roughly two or three weeks. In case you had time to investigate a bit more already (only if tackling weirdness in T314 didn't keep you too busy), could you elaborate where the challenges lie (or point me to upstream comments in case I missed them)? Is this next after T422 and T404, or do you see something else that's has higher priority currently? Thanks! :)

mmodell wrote on 2014-07-02 17:02:34 (UTC)

@Aklapper: I've already started the code for this one. It's not that difficult it just took a while for me to get my head around the different pieces of phabricator that this touches. I don't think it'll be more than a week or two to have this finished and it doesn't go through upstream so the review process will be a little simpler.

Rush wrote on 2014-07-02 17:31:57 (UTC)

@mmodell, are we wrapping up already upstream stuff or going our own way? I thought the former, sounds like the latter?

mmodell wrote on 2014-07-04 00:12:16 (UTC)

The latter, I'm building a plugin to enforce visibility of tasks in the security project.

Rush wrote on 2014-07-04 22:06:42 (UTC)

@mmodell...kinda worried about this new approach man. Part of ops signing on was us pushing through the seecurity features needed upstream. If we go our own way it is a large burden going forward having to solely vett complex interactions in the large and constantly changing code base. I'm not sure who would be suitable to review this even? There are some work flows I think covered by the upstream tasks that may not be obvious too. In my view doing something this important outside of upstream is a big topic we need to hash out. Why the change in strategy? Hoping we can all discuss this out next week, but I don't know if this works out for ops especially.

mmodell wrote on 2014-07-04 22:12:20 (UTC)

@rush, this isn't a new approach. The upstream support for this is not finished and it's also not really directed at our use case. The code is straight-forward to do it as a plugin, and I don't think it's going to require any messy patches. Once upstream has the infrastructure in place then we can look into using that but otherwise it's going to be a large roadblock that could hold us up for quite a while.

Rush wrote on 2014-07-04 22:26:19 (UTC)

@mmodell. By new I guess I meant outside of the context of upstream. I'm questioning whether we should direct energy upstream then? I do understand the work upstream is TBD on timeline but we were all on the same page for directing our efforts towards finishing it as far as I knew. Can we set aside some time this week with all of us to talk about what this will look like / how it will work? BTW not trying to sap your 4th man this can wait, I just couldn't stop worrying about it until I put something in writing. I feel like you may spend a lot of time on this and we won't be able to use it. Anyway, have a good weekend.

mmodell wrote on 2014-07-04 22:30:09 (UTC)

@rush if you look at https://secure.phabricator.com/T3820 you will see that it's really not going to address our use case and we'd probably STILL need to do some sort of custom coding.

If you aren't comfortable with any custom code in phabricator then why are we even using phabricator? Might as well buy some closed source thing then if we don't plan to customize it any.

mmodell wrote on 2014-07-04 22:45:21 (UTC)

epriestley's advice was to make a small patch and not try to do this upstream. Honestly upstream is not very interested in our use case, to the point of being almost hostile to it. Read over the "namespaces" stuff carefully and you'll see it's way more heavyweight than what we need and it doesn't even provide proper support for simply hiding tasks from the public. On top of that it's months away from being really ready.

It's not that much work to write something custom. The existing policy/capability framework does exactly what we need, it's just a matter of attaching a little magic behavior to a specific project so that the default visibility of tasks gets overridden.

Most of the complexities involve the interaction of policies from multiple projects... But we don't need to support that really, we can say that "security report" is the only "magical" project and it'll be the only one with default policies, so it won't have to interact with any other projects.

I really don't understand how ops could have any grounds for concern about this:

1. It's only slightly more complex than https://gerrit.wikimedia.org/r/#/c/143506/
2. It's way less complex than https://secure.phabricator.com/D9202
3. We have already merged (2.) prior to getting upstream review, and that didn't seem to raise any red flags for anyone.

Having said all of that, I'm more than willing to have a discussion about it, here or elsewhere. But by the time next week is here, the code for this will be finished and the upstream support will still be months away and unsuitable for our specific use-case.

mmodell wrote on 2014-07-07 19:49:35 (UTC)

http://fab.wmflabs.org/P11

This little bit of code automatically sets the tickets to the same policy as the project when assigning a ticket to a "special" project.. the list of special projects is hard-coded.

mmodell wrote on 2014-07-08 00:27:24 (UTC)

This is now live on this labs instance for testing. and it's submitted for code review at https://gerrit.wikimedia.org/r/#/c/144620/

mmodell wrote on 2014-07-08 00:29:50 (UTC)

The relevant project is http://fab.wmflabs.org/tag/security/

mmodell wrote on 2014-07-08 15:09:12 (UTC)

As you can see, T198 has been added to the security project, and if you are not a member of that project, you cannot see T198

qgil wrote on 2014-07-09 15:07:49 (UTC)

I just found out by accident, after filing T440:

1. A task filed without any project declared is assigned automatically to the Security project. This shouldn't be the case.
2. When a task is filed under the Security project, the reporter can't see it either. That was the case when I created T440.

Other than that, T440 became quite invisible after it moved under the Security umbrella. I tried to list it as a blocker of another task (since I knew the task number) but Phabricator wouldn't list it. I also tried to call it with curly brackets: T440: Verify with Ops (Marc) the resourcing needs for another Beta Cluster-type project in WMF Labs and the subject wouldn't be spelled either.

mmodell wrote on 2014-07-09 16:33:49 (UTC)

I'm not sure how a task is automatically assigned to the security project. that seems really odd.

mmodell wrote on 2014-07-09 16:35:41 (UTC)

I just submitted T446 and it didn't get assigned to the security project.

qgil wrote on 2014-07-09 17:09:53 (UTC)

I can't access T446

Rush wrote on 2014-07-09 17:16:16 (UTC)

mukunda it wasn't the project it was "viewable by" that was set to Security. should have made that clearer sorry!

mmodell wrote on 2014-07-09 23:42:29 (UTC)

@Qgil @rush ok I found the bug and fixed it, no more defaulting to the wrong policy.

qgil wrote on 2014-07-10 11:13:07 (UTC)

In T95#66, @mmodell wrote:

@Qgil @rush ok I found the bug and fixed it, no more defaulting to the wrong policy.

Tested at T448. Correct!

mmodell wrote on 2014-07-11 05:28:44 (UTC)

Ok I got a detailed response from Evan about different approaches to this one. See https://secure.phabricator.com/T5571

mmodell wrote on 2014-07-15 17:02:54 (UTC)

Mostly finished re-writing this as a custom herald action, which looks like it will work really well.

mmodell wrote on 2014-07-18 00:47:27 (UTC)

Doesn't seem like upstream is interested in this one. I'm going to submit it to our own phabricator extensions repository, I don't see any other avenue to publishing this work.

So, question... How should we license phab extensions that aren't upstreamed?

mmodell wrote on 2014-07-18 11:20:00 (UTC)

The herald custom action is now installed on this instance and the new herald rule that uses it is H5

mmodell wrote on 2014-07-22 00:12:31 (UTC)

So I think this one is essentially done though I'm still waiting on code review in gerrit: https://gerrit.wikimedia.org/r/#/c/147356/

mattflaschen wrote on 2014-07-26 01:25:52 (UTC)

In T95#56, @mmodell wrote:

epriestley's advice was to make a small patch and not try to do this upstream. Honestly upstream is not very interested in our use case, to the point of being almost hostile to it. Read over the "namespaces" stuff carefully and you'll see it's way more heavyweight than what we need and it doesn't even provide proper support for simply hiding tasks from the public.

It seems like it does. Per https://secure.phabricator.com/T3820#16 , we would need a Security namespace, and then give the security team only access to that namespace (except that reporters could report bugs into it, and view and comment on their own bugs).

I do think for something as important as security, it's better to have it baked in upstream, so people are always aware of it when working on the main project.

However, a plugin may be appropriate as a temporary approach until upstream has what we need.

mattflaschen wrote on 2014-07-26 01:28:09 (UTC)

In T95#70, @mmodell wrote:

So, question... How should we license phab extensions that aren't upstreamed?

If it uses the same license as Phabricator (Apache v2), it's possible to get code from upstream if needed. Contributing the other way requires signing a CLA regardless.

Rush wrote on 2014-08-18 18:00:40 (UTC)

Changed the title to reflect a more consistent approach considering how phabricator works. We set the view/edit permissions on the ticket itself, ideally in this case to 'people' groupings. So instead of projects owning tasks -- tasks are associated via perms settings to projects. It's the same end result, but opposite thinking.

Rush wrote on 2014-08-20 15:12:03 (UTC)

This is the relevant extension in the works now:

https://gerrit.wikimedia.org/r/#/c/154850/

mmodell wrote on 2014-09-01 22:34:50 (UTC)

@rush I've updated the patch at https://gerrit.wikimedia.org/r/#/c/154850/ - it's been tested locally and seems to be working as far as I can tell. Further testing is needed but I think it's about ready..

Rush wrote on 2014-09-06 14:35:17 (UTC)

should work. two levels of resriction, operations group and security group.

http://git.wikimedia.org/commit/phabricator%2Fextensions.git/1f7363a72897499ba0d9c551e362cd4123abfd03

Rush wrote on 2014-09-06 14:35:31 (UTC)

should work. two levels of resriction, operations group and security group.

http://git.wikimedia.org/commit/phabricator%2Fextensions.git/1f7363a72897499ba0d9c551e362cd4123abfd03

aklapper wrote on 2014-09-08 16:44:03 (UTC)

Thanks everybody who worked hard on this. Great to see this fixed.

importing issue status

@mmodell, is everything setup here to handle the private tasks needed by RT and Bugzilla? Can we resolve this task?

I created T474 and I'm referencing here to see what happens: {T474} (so far looks good, no subject leaked). Can you please change the security settings to "None", in order to see what happens?