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 any luck? Most of us are going to have slim availability starting Friday 9/23 FYI.

In T146055#2656048, @chasemp wrote:

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

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

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

3. I added the phd user to the www-data group so that the daemons can read local.json

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) (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

In T146055#2801400, @mmodell wrote:

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

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.

should this ticket have DBA tag?

In T146055#3104429, @Dzahn wrote:

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.

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

https://gerrit.wikimedia.org/r/620822

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

https://gerrit.wikimedia.org/r/620822

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

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

https://gerrit.wikimedia.org/r/620879

This is done: https://gerrit.wikimedia.org/r/c/operations/puppet/+/620879 Anything pending other than merging that?

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

https://gerrit.wikimedia.org/r/620879