Page MenuHomePhabricator

Arcanist security review (before being used in WMF deployments)
Closed, DeclinedPublic

Description

bd808 wrote on 2014-04-28 22:32:24 (UTC)
The "train deploy" system currently in use by the WMF replies pretty heavily on submitting small patches from the deployment staging server running in the cluster to gerrit. This is done today using git push commands to the appropriate magic branches in Gerrit. If arc becomes required for submitting patches for review we would have to do a full security audit of arc to determine if it could be deployed to the cluster.

Event Timeline

Qgil created this task.Oct 10 2014, 8:29 AM
Qgil raised the priority of this task from to Low.
Qgil updated the task description. (Show Details)
Qgil changed Security from none to None.
Qgil added subscribers: Unknown Object (MLST), Qgil, bd808.
In T127#1803, @flimport wrote:

hashar wrote on 2014-04-29 08:49:01 (UTC)
For production we can still use the plain git repository and fetch from it. Seems to me arc will not be required to update the local repositories on our deployment servers.

In T127#1804, @flimport wrote:

mattflaschen wrote on 2014-04-29 22:47:17 (UTC)
hashar:

For production we can still use the plain git repository and fetch from it. Seems to me arc will not be required to update the local repositories on our deployment servers.

I think bd808 is referring to pushing from the production servers, not fetching to them. For example, bumping the submodule for the extension. This can be done locally, but I think some people may do it from the deployment servers.

Qgil moved this task from Need Discussion to To Triage on the Gerrit-Migration board.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 24 2015, 7:03 PM
greg raised the priority of this task from Low to Normal.Sep 9 2015, 3:13 PM
greg updated the task description. (Show Details)
csteipp added a subscriber: csteipp.Sep 9 2015, 3:57 PM

What is the timeline for this?

greg renamed this task from Arcanist needs a security review before being deployed to the Wikimedia cluster to Arcanist security review (before being used in WMF deployments).Sep 9 2015, 4:11 PM
greg added subscribers: mmodell, greg.

What is the timeline for this?

@mmodell: We probably won't use arcanist on tin.wmnet with our MW deploys (to talk with Differential instead of Gerrit) (or other deploys) in Q2 right? Probably Q3?

Implementing differential does not necessitate that we use arc for deployments. Or, what @greg said.

greg lowered the priority of this task from Normal to Lowest.Sep 9 2015, 4:19 PM

Changing priority accordingly (and removing the Gerrit-Migration project since this (being able to use arcanist in production). isn't a blocker, but a nice to have). Will re-prioritize if we deem it something we want, but we'll keep Security Review turn around time in mind :)

Carry on!

The copy of arcanist we use will always need to be in sync with the phabricator HEAD we use on phabricator.w.o at the time. Will each update of our copy of arcanist then be security reviewed before its merged?

Besides distribution provided software this will be the one additional thing that we probably need to run on the same system as production ssh keys. Can the person doing the security review gpg sign each push to our arcanist git repo?

Does arc as is automatically update itself and run code whose signature is not yet checked?

greg added a comment.Sep 9 2015, 4:32 PM

The copy of arcanist we use will always need to be in sync with the phabricator HEAD we use on phabricator.w.o at the time. Will each update of our copy of arcanist then be security reviewed before its merged?

That's not how security reviews work generally (but Chris can correct if I'm wrong), we don't do a security review of each new change/update to MW extensions, for instance. :)

greg added a comment.Sep 9 2015, 4:56 PM

Just keeping this task filled with the discussion from IRC:

16:36 < jzerebeck> greg-g: so if I understand that right i will need to use arc to submit patches, on the same machine as my 
                   production key, before we trust it enough to run it in this same production?
16:37 <+   greg-g> jzerebecki: we don't do security reviews of pidgin or firefox or whatever else is on your local machine that has 
                   your prod keys
16:37 <+   greg-g> it's a red herring
16:38 <   Krenair> or git-review
16:38 <   Krenair> or git itself
16:38 <+   greg-g> heh, touche
16:39 <   Krenair> I don't know if they review some ssh client version or not, but I'm pretty sure they don't review every version in 
                   use on all machines with some sort of production key
16:45 < jzerebeck> greg-g: those are signed by debian people
16:48 < jzerebeck> greg-g: i don't usually curl random.com | bash  even though most people do
16:50 <+   greg-g> jzerebecki: I think this issue is really "do what we do with git-review" and since we haven't internally reviewed 
                   git-review (we trust upstream) then we don't need to review Arcanist. I'll let chris chime in if he disagress 
                   though, of course :)

Summary: definite security if we use arcanist on tin (not currently planned), security review if Chris wants one because it'll be installed on deployers' personal laptops (a la git-review).

I'm not saying don't trust upstream Arc. We trust Debian. What we can do with Debian packages, we should also do with PHP code. We should also have other organisations / people we trust. Trustworthy ones exist outside the mediawiki gerrit group. But that requires knowing that what you get is what upstream intended. See T105638 for context, there with composer instead of plain git.

For code we execute on the Wikimedia cluster, currently we rely on transport security to gerrit.w.o and on code integrity by gerrit.w.o. If a security review of Arc for executing it on the cluster is useful and necessary for a defense against a threat. Then, to not create a weak link in the defense, it is necessary to have a security review of the code integrity function Differential with phabricator.w.o will have for us. Is my reasoning sound, @csteipp?

Empty output means not signed:

phacility/arcanist (master=)$ git verify-commit -v origin/master
phacility/arcanist (master=)$ git verify-tag -v conduit-5
error: no signature found
phacility/arcanist (master=)$ git verify-tag -v conduit-6
error: conduit-6: cannot verify a non-tag object of type commit.
phacility/arcanist (master=)$ git verify-commit -v conduit-6
Legoktm added a subscriber: Legoktm.Sep 9 2015, 8:20 PM

For the record, git-review isn't installed on tin. Deployers use pure git to push to gerrit.

FWIW, The Arcanist upstream code changes fairly infrequently and those changes usually come in easy to review, bite-sized pieces.

We host our own Arcanist repository and there is a self-update command, arc update which simply does a git pull from origin (origin should point to our fork, not upstream.) Arcanist is also a fairly widely used tool, with a clean code base, written by very competent developers, within a highly structured framework. As far as I can see, arc isn't even the slightest bit guilty of the kind of reckless approach to security that is seen in composer.

greg moved this task from To Triage to Backlog on the Differential board.Nov 4 2015, 7:04 PM

Just following up on old security review tasks.

AFAICT, the transition to arc seems pretty stalled in general. Is there still a security review needed here, or should I close this ticket?

@Bawolff: T119908#4033583 implies that there are no plans to move from Gerrit to Differential (Differential uses Arcanist). I guess this task could be declined.

Bawolff closed this task as Declined.Mar 13 2018, 11:22 AM

Ok. Please feel free to reopen if the situation changes.