Page MenuHomePhabricator

Improve privilege separation for phabricator's config files and mysql credentials
Closed, ResolvedPublic


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.

Revisions and Commits

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.

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)

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. 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.
  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

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?

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

"Enable multiple config files in phabricator" (20after4) (
"rename iridium-vcs to phab1001-vcs" (Dzahn) (
"Move config for git-ssh(phabricator) to hiera" (20after4) (
"conftool/phabricator: replace iridium-vcs with phab1001-vcs" (Paladox) (
"phabricator: PHABRICATOR_ENV for vcs service" (20after4) (
"phabricator: make vcs user a member of phd group" (20after4) (
"phabricator: fix one stray PHABRICATOR_ENV=vcs" (20after4) (

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,;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 revision: Restricted Differential Revision.Nov 18 2016, 4:47 PM
Aklapper closed subtask Restricted Task as Resolved.Nov 25 2016, 10:50 AM
mmodell lowered the priority of this task from High to Medium.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.
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...

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.

@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.

Paladox removed mmodell as the assignee of this task.
Paladox updated the task description. (Show Details)
Paladox removed a subscriber: dpatrick.

The next step here is to create a $passwords::mysql::phabricator::phd_pass and update the phabricator manifest to use that password for phd. Once the password exists, it's a simple change to make phabricator use it for the phd daemon processes. See rOPUP /modules/profile/manifests/phabricator/main.pp$162

I've created $passwords::mysql::phabricator::phd_pass on the private repo, but not sure which mysql user this will correspond to?

Change 620822 had a related patch set uploaded (by 20after4; owner: 20after4):
[operations/puppet@production] Phabricator: use a separate mysql user for phd daemons

Change 620822 merged by Jcrespo:
[operations/puppet@production] Phabricator: use a separate mysql user for phd daemons

Mentioned in SAL (#wikimedia-operations) [2020-08-18T06:21:13Z] <jynus> deploy password change to phabricator service T146055

jcrespo changed the task status from Stalled to Open.Aug 18 2020, 7:47 AM

Change 620879 had a related patch set uploaded (by Jcrespo; owner: Jcrespo):
[operations/puppet@production] phabricator: Add new db user for the daemon, separate from web request user

Change 620879 merged by Jcrespo:
[operations/puppet@production] phabricator: Add new db user for the daemon, separate from web request user

Marostegui assigned this task to jcrespo.