Page MenuHomePhabricator

Improve privilege separation for phabricator's config files and mysql credentials
Open, Stalled, NormalPublic

Description

Phabricator's config file is world readable:

-rw-r--r-- 1 root        root        7120 Sep 14 00:42 local.json

This file needs to be read by the web server process and by the phabricator daemons.

  1. We can improve this slightly by making phd a member of the www-data group and changing this file to 640.
  2. We can further improve the situation by sandboxing repository operations to a mysql account that only has access to phabricator_repository and phabricator_daemon databases.

We can put different mysql credentials in different config files by using phabricator config environments ( described here ). Then we can have different permissions on each version of the config file.

Event Timeline

mmodell created this task.Sep 19 2016, 5:05 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 19 2016, 5:05 PM
greg added a subscriber: greg.Sep 19 2016, 5:13 PM

A small note, this is the app_pass under passwords::mysql::phabricator which is for the phuser acccount and not phadmin. We'll want to coordinate with @jcrespo to change this pretty soon here. Thanks @20after4

Just tell me when so I can make sure client and server change the password at the same time. By experience, it usually requires connection closing as phabricator makes usage of heavy connection pooling.

@chasemp, @jcrespo: it's not just about changing the password, I think we need to add a new mysql account for PhabricatorRepositoryPullLocalDaemon, which can be more limited than phuser. At least that seems like the best way to segment the repository operations from potential privilege escalation in case of an attack on git.

It looks like the repository daemon only needs access to read/write on phabricator_repository and phabricator_daemon databases. So we could, at least in theory, run the repository daemons on a separate mysql account that is limited to just those.

To deal with the permissions on the config file we probably need to add the phd user to the www-data group and then chmod 640 phabricator/conf/local/local.json

mmodell renamed this task from phabricator config (local.json) is world-readable and contains mysql password to Improve privilege separation for phabricator's config files and mysql credentials.Sep 19 2016, 7:51 PM
mmodell updated the task description. (Show Details)

@mmodell any luck? Most of us are going to have slim availability starting Friday 9/23 FYI.

@mmodell any luck? Most of us are going to have slim availability starting Friday 9/23 FYI.

friendly ping :)

@chasemp: sorry I seem to have let this one slip, I will see if I can put together a patch for this.

mmodell added a subscriber: Dzahn.Nov 15 2016, 7:48 PM

I think this is all ready to go but I'd appreciate some code review on the puppet patch. The supporting changes in phabricator will be deployed tomorrow during the scheduled upgrade: Phabricator (2016-11-16)

mmodell added a comment.EditedNov 15 2016, 8:01 PM

So here's a high-level view of what I've done with the linked changes:

  1. In 41a7167f9cfb and f38f9e13fd69, I Slightly patched phabricator to support reading from multiple json config files based on the environment variable PHABRICATOR_ENV. The default behavior of PHABRICATOR_ENV previously caused phabricator to look for a php file that returns an array. json is cleaner and the patch to phabricator core is clean / safe.
  2. https://gerrit.wikimedia.org/r/#/c/321654/ Moves the mysql configuration parameters to separate conf/local/phd.json and conf/local/www.json so now we have 3 config files which have permissions as follows.
fileusergidmode
local.jsonrootwww-data0644
www.jsonrootwww-data0640
phd.jsonrootphd0640
  1. I added the phd user to the www-data group so that the daemons can read local.json
daemon-config maplocal.jsonphd.jsonwww.json
phd daemons
apache/php

So this solves the world-readable problem and it will allow us to use separate credentials for phd and www.

Ok all that remains now is to create a new password, change the password in mysql, and put it in puppet/private.git

I believe we need help from a dba for the mysql part. @jcrespo?

Dzahn added a comment.Nov 17 2016, 6:27 AM

ACK, we reviewed, amended, merged and deployed together:

"Enable multiple config files in phabricator" (20after4) (https://gerrit.wikimedia.org/r/#/c/321654/)
"rename iridium-vcs to phab1001-vcs" (Dzahn) (https://gerrit.wikimedia.org/r/#/c/317290/)
"Move config for git-ssh(phabricator) to hiera" (20after4) (https://gerrit.wikimedia.org/r/#/c/318662/)
"conftool/phabricator: replace iridium-vcs with phab1001-vcs" (Paladox) (https://gerrit.wikimedia.org/r/#/c/322034/)
"phabricator: PHABRICATOR_ENV for vcs service" (20after4) (https://gerrit.wikimedia.org/r/#/c/322041/)
"phabricator: make vcs user a member of phd group" (20after4) (https://gerrit.wikimedia.org/r/#/c/322042/)
"phabricator: fix one stray PHABRICATOR_ENV=vcs" (20after4) (https://gerrit.wikimedia.org/r/#/c/322044/)

nice progress and team work

I believe we need help from a dba for the mysql part. @jcrespo?

This applies now: T146055#2649519 :-)

@jcrespo: indeed, https://phabricator.wikimedia.org/diffusion/OPUP/browse/production/modules/role/manifests/phabricator/main.pp;057bc7cb238c4c55e2a33e93eed65c99e5b95057$41 is where the username/pasword is set. Currently the two config files have the same username/password but it's a simple thing to change and store the new credentials in puppet/private.git

mmodell added a subtask: Restricted Task.Nov 21 2016, 6:04 AM
Aklapper closed subtask Restricted Task as Resolved.Nov 25 2016, 10:50 AM
mmodell lowered the priority of this task from High to Normal.Dec 5 2016, 1:49 PM

I'm removing the security policy on this task as the file with credentials is no longer world readable. This task only remains open because we still need to set up separate sql account usernames for the various phabricaor daemons/processes.

mmodell changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 5 2016, 1:51 PM
mmodell changed Security from Software security bug to None.
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptDec 5 2016, 1:51 PM
Paladox moved this task from To Triage to Misc on the Phabricator board.Feb 17 2017, 4:57 PM
jcrespo added a parent task: Restricted Task.Feb 20 2017, 4:39 PM
mmodell changed the task status from Open to Stalled.Mar 15 2017, 9:51 PM
mmodell removed mmodell as the assignee of this task.

unassigning as there is nothing more I can do here.

should this ticket have DBA tag?

should this ticket have DBA tag?

Yeah probably so...

1978Gage2001 moved this task from Triage to In progress on the DBA board.Dec 11 2017, 9:45 AM
Marostegui moved this task from In progress to Triage on the DBA board.Dec 11 2017, 11:06 AM

Upon further reflection, I suspect that it won't work to sandbox the repository operations to just those databases - operations on repositories might trigger other operations which need to touch other databases such as closing tasks when the closes: Tnnn action is detected in a commit message. It would be difficult to isolate all the possible databases that a repo daemon might need and I suspect we wouldn't gain much security from such isolation after all.

We need to catch up on a lot of pending DBA-phabricator tasks (this, some pending failovers, upgrades to strech/mariadb 10.1) and proxyies, codfw deplyment. Let's try to schedule some time to do all of those, but for now it will have to wait as we have other important fires going on.

mmodell claimed this task.Feb 17 2018, 6:49 PM

@jcrespo: just let me know when you are available, I'll be glad to get to work with you on any phabricator tasks.

I am now more available, this should be added to the lists of things to discuss.

jcrespo moved this task from Triage to Backlog on the DBA board.Jun 15 2018, 3:15 PM
Aklapper moved this task from Misc to Database fiddling on the Phabricator board.Aug 29 2018, 6:05 PM
Paladox reopened this task as Stalled.Feb 2 2019, 7:32 PM
Paladox removed mmodell as the assignee of this task.
Paladox updated the task description. (Show Details)
Paladox removed a subscriber: dpatrick.