Page MenuHomePhabricator

Security review of EventBus extension
Closed, ResolvedPublic

Description

As part of the effort to build an Event-Platform system for MediaWiki, we are working on an extension which will emit basic MediaWiki events. We are seeking input from the Security-Team to review its security aspects. All of the extension's code can be found in Gerrit PS 254086.

See Also

Event Timeline

mobrovac created this task.Dec 3 2015, 1:03 PM
mobrovac raised the priority of this task from to High.
mobrovac updated the task description. (Show Details)
mobrovac added subscribers: mobrovac, csteipp, Dereckson and 4 others.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 3 2015, 1:03 PM

For background, all of the consumers of the eventbus are trusted systems (inside the WMF cluster in our case), right?

For background, all of the consumers of the eventbus are trusted systems (inside the WMF cluster in our case), right?

Yes, at this stage all of the producers (including this extension) and consumers are internal.

Yes, at this stage all of the producers (including this extension) and

consumers are internal.

So.. plans to have external consumers in the future? Or is the future
undecided at this point? It makes a big difference from a security
perspective, so I think that needs to be established soon, in either case.

If there are external consumers in the future, they will be similar to RCStream, and made for specific use cases. There may be external consumers built off of the data that will be generated by this extension, but they will just be consumers of data in Kafka.

Additionally, the extension itself will never directly talk to external clients, only to the service validating and proxying event messages to Kafka (all internal).

Can someone link to the proposed consumers and a system-level design for this?

"system-level design" = what systems will the data be flowing into, and for (roughly) what purposes?

Eevans added a comment.Dec 4 2015, 7:13 PM

Can someone link to the proposed consumers and a system-level design for this?

"system-level design" = what systems will the data be flowing into, and for (roughly) what purposes?

RESTBase is the only current use-case/consumer for these events, and initially this will only cover the same transitions in state that RestbaseUpdateJobs currently provides. For changes to revision visibility, this is encapsulated in this event schema definition.

My understanding is that username suppressions will be added at some point in the future, when there is a use/need for them on the RESTBase end of things (or elsewhere). @mobrovac or @GWicke can probably elaborate more on this though.

For changes to revision visibility, this is encapsulated in this event schema definition.

So the issue I'm worried about is in this schema, you have,

user:
   type: boolean
   description: "whether the author of the revision's text is available"

If by this, you're saying, "whether the author of the revision's text should be shown", in MediaWiki this is determined by 2 factors-- if the username has been delete/suppressed on a particular revision, or if the username has been suppressed on the entire wiki (using a suppression block). If Restbase is going to cache the username, then it needs to be aware of both. The patch that @mobrovac merged only accounts for the first.

Eevans added a comment.Dec 4 2015, 7:50 PM

If by this, you're saying, "whether the author of the revision's text should be shown", in MediaWiki this is determined by 2 factors-- if the username has been delete/suppressed on a particular revision, or if the username has been suppressed on the entire wiki (using a suppression block). If Restbase is going to cache the username, then it needs to be aware of both. The patch that @mobrovac merged only accounts for the first.

Aha! Thanks for your patience, I think I get it now. :)

Ok, so that would be something for a separate event, I assume. And, it's something we don't currently support in RESTBase (i.e. we wouldn't currently hide the author name for any such wiki-wide deletion/suppression). But we should, so I'll open a separate issue to track that.

Eevans added a comment.Dec 4 2015, 7:59 PM

... But we should, so I'll open a separate issue to track that.

For reference: T120409: RESTBase should honor wiki-wide deletion/suppression of users

... But we should, so I'll open a separate issue to track that.

For reference: T120409: RESTBase should honor wiki-wide deletion/suppression of users

@GWicke - can you let me know what the timeline for implementing this will be? I'm not currently comfortable closing this security review with this gap, unless we have a clear roadmap/timeline for fixing it.

@csteipp: Unless I'm missing something here, (which is possible), this isn't a regression; These wiki-wide suppressions aren't currently covered by RESTBase (presently the only use-case/consumer for these events). And, it's not information missing from the revision_visibility_set event, this would require the definition of a separate, distinct event. That event will need to be created in order to complete T120409, (or for any other consumer that needs that it), but doesn't (again, unless I'm missing something), change the implementation here.

@csteipp: We'll prioritize adding support for global user suppression early next quarter.

We are hoping to start moving the extension into production soon; Are we still treating the absence of wiki-wide user suppression as a blocker here?

csteipp added a comment.EditedJan 12 2016, 9:08 PM

I don't consider the system meeting our security standards until T120409: RESTBase should honor wiki-wide deletion/suppression of users is fixed. @GWicke says that will be fixed in January.

So as long as restbase is the only consumer of this, and the team is committing to fixing T120409 this quarter, go ahead and deploy it, and we'll close this task when the blocker is fixed.

@csteipp: As mentioned on IRC, we will prioritize this & come up at least with a temporary solution early this quarter. Revision metadata end points are low volume at this point, which lets us consider simple solutions.

Eevans added a comment.EditedJan 13 2016, 10:43 PM

/page/revision/{revision} and /page/title/{title} would seem to be the only places we currently risk a leak. The following PR disables RESTBase caching of revision metadata as a temporary stop-gap, until a more permanent solution is put in place.

https://github.com/wikimedia/restbase/pull/468

The PR has been merged. To be deployed shortly.

With PR 468 deployed, RESTBase queries the MW API for each content request and hence denies access for edge cases. @csteipp, @dpatrick can we now close this?

With PR 468 deployed, RESTBase queries the MW API for each content request and hence denies access for edge cases. @csteipp, @dpatrick can we now close this?

As long as RESTBase is the only consumer of the eventbus, then this solution works. I've heard other teams saying they want to use it, and they would need to account for this also. Is that documented, or is there a check in the process to make sure other teams do that?

Is there an eta on T120409 yet?

As long as RESTBase is the only consumer of the eventbus, then this solution works. I've heard other teams saying they want to use it, and they would need to account for this also.

Per normal policy, any service exposing EventBus (or more generally sensitive) information to the public would need to go through its own security review. This isn't different from RCStream or PHP extensions.

GWicke added a comment.EditedMar 1 2016, 9:43 PM

Is there an eta on T120409 yet?

T120409 is resolved for all current / new content.

The only exception is that older, stored Parsoid content still has references to user names in its HTML head, which we will strip in a post-processing step. This isn't really related to EventBus, it just came up in the context of this conversation.

Is there an eta on T120409 yet?

T120409 is resolved for all current / new content.
The only exception is that older, stored Parsoid content still has references to user names in its HTML head, which we will strip in a post-processing step. This isn't really related to EventBus, it just came up in the context of this conversation.

Sorry, I must have misunderstood the intent of that bug. Since it (initially) had T122079 as a subtask, I assumed that meant the eventbus would push block events, including suppression blocks, so that consumers would get notifications of suppressions. Is there no plan to do that?

GWicke added a comment.EditedMar 2 2016, 6:15 PM

Sorry, I must have misunderstood the intent of that bug. Since it (initially) had T122079 as a subtask, I assumed that meant the eventbus would push block events, including suppression blocks, so that consumers would get notifications of suppressions. Is there no plan to do that?

The medium term plan is still to provide user deletion / suppression events in EventBus, which we can then use to safely expose user information from services at a later time.

So, as far as I can tell, all EventBus MVP related work is done. This ticket is still open because T120409 is related, but this has nothing to do with EventBus. I really think we should remove that as a blocking task and close these. @csteipp, objections?

So, as far as I can tell, all EventBus MVP related work is done. This ticket is still open because T120409 is related, but this has nothing to do with EventBus. I really think we should remove that as a blocking task and close these. @csteipp, objections?

The real blocker is T122079. Until then, anything using the eventbus has to figure out their own way of handling user suppressions if they store usernames anywhere.

@Ottomata, what is your specific use case?

From my PoV, the EventBus MVP had to do with standing up the service and receiving a minimal set of events from MW. This work was completed 2 quarters ago, so I'd just like to be able to close the ticket.

@Ottomata, I understand the desire. Unfortunately, T122079 is a design flaw (from my understanding of the problem, but happy to discuss that) that should have been addressed before the service was put into production. I'm not going to close the security review until it's fixed. Maybe @GWicke or @Eevans can comment on the timeline?

@csteipp As of 1.27.0-wmf.22 user block events are being emitted to the EventBus. Given that T120409 is effectively resolved for latest content, can we now consider this task as completed?

csteipp closed this task as Resolved.Apr 27 2016, 7:00 PM
csteipp claimed this task.
Restricted Application added a project: Analytics. · View Herald TranscriptJul 12 2017, 5:27 PM
sbassett moved this task from Backlog to Done on the Security-Team board.Jun 11 2019, 7:12 PM