Page MenuHomePhabricator

Start phd daemons as the correctly configured user and refuse otherwise
ClosedPublic

Authored by fabe on Dec 23 2014, 12:49 PM.

Details

Summary

Fixes T5196
If no phd.user is configured the behaviour is unchanged besides printing a warning when run as root (Usually i would add an exit(1) here but that would break existing installs who do that).
If phd.user is set and the current user is root it will run the daemon as: su USER -c "command" (I'm not sure if this works for every platform needed)
Otherwise it will refuse to start if configured and current user mismatch.

Test Plan

Stopped & Started phd daemon with various users and different phd.user settings including root

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fabe updated this revision to Diff 26497.Dec 23 2014, 12:49 PM
fabe retitled this revision from to Start phd daemons as the correctly configured user and refuse otherwise.
fabe updated this object.
fabe edited the test plan for this revision. (Show Details)

A couple of notes inline.

One weakness of this approach is that phd debug will still run as the current user. Once phd.user is properly enforced, I think we should require users to pass a flag like --as-current-user to do this -- for example:

$ phd debug taskmaster
PhutilArgumentUsageException: You are trying to run a daemon as a nonstandard user,
and `phd` was not able to `sudo` to the correct user. Phabricator is configured to run
daemons as "phdaemon", but the current user is "yourusername". Use `sudo` to run as
a different user, pass `--as-current-user` to ignore this warning, or edit `phd.user`
to change the configuration.
$ phd debug taskmaster --as-current-user
Warning: Running as nonstandard user.
blah blah normal output

A broader concern I have about this is that it may break some installs in a way that's hard for them to figure out. Specifically, if they have phd.user configured but do not start the daemons as either that user or root, daemons which may have worked previously will suddenly stop working.

I think this is the right behavior in the long run, but we should make more of an effort to warn installs before pursuing it. One approach might be:

  • If --as-current-user is not present:
    • If phd.user is set, and the current user is not the correct user:
      • If the command is debug, throw the exception above.
      • If the command is not debug, try to sudo. If sudo fails:
        • Today: Emit a warning to the console and continue (run the command again without sudo).
        • Later: Throw a similar exception.

This shouldn't break anything, but this warning will be very easy to miss, since many existing installs probably do not look at this command's output. To make it more clear and make sure installs know about the problem, we can add a runningAsUser column to PhabricatorDaemonLog, and record it in PhabricatorDaemonEventListener->handleLaunchEvent().

Finally, add a SetupCheck to look for daemons running as the wrong user (this can go in PhabricatorSetupCheckDaemons, which has similar checks) and raise a warning if any are found.

This should tell administrators about the issue and give installs more flexibility and better behavior, and then in a few months we can make phd throw (instead of continuing) when sudo fails.

Sound reasonable?

src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
207

Use %s instead of "%C" -- %s will automatically apply proper escaping, but "%C" will produce the wrong result if $command already contains a quote (or various other special characters).

You should also use %s for the "run as user", since it's possible that phd.user might be set to `rm -rf /` or similar (with backticks, so it executes a command if used unquoted).

This would be a bit cleaner as sudo -En -u %s -- %s, which makes sure the environment is preserved and allows administrators to configure this to work through sudoers instead of needing to start the daemons as root (it looks like su also has -m to preserve the environment, but it's less configurable).

339–342

Prefer to throw new PhutilArgumentUsageException(...) to indicate that a user has misused or misconfigured a command. This will give the program more flexibility in shutting down than exit(1).

fabe added a comment.Dec 23 2014, 2:22 PM

Sounds good. I'm not quite sure about the "if sudo fails" part though as i can't really differentiate between sudo failing and the phd daemon failing for another reason.
Worst that can happen though is that the daemon fails to start twice.
The problem with the %s for the command is though that it was built in the csprintf just before and the double escaping will produce things like ('./phd-daemon '\''PhabricatorRepositoryPullLocalDaemon'\''). %C for the command should be safe though.

For now, I think it's OK to act like any error from sudo is a sudo failure. If it was really a daemon failure, the automatic re-execution of the command afterward should show the real error anyway.

%C for the command should be safe though.

Ah, you're right.

to support selinux context switches you should use runuser instead of su.

http://danwalsh.livejournal.com/55588.html

see also my runuser invocation in rhel init script: https://github.com/vinzent/phabricator/blob/master/resources/rhel/phabricator.init#L42

fabe updated this revision to Diff 26499.Dec 23 2014, 3:17 PM
fabe edited edge metadata.

use sudo and fix debug. also try without sudo if it fails.
Log runningAsUser to the daemon log

fabe added a comment.Dec 23 2014, 3:18 PM

I think using runuser would limit the portability quite a bit and most admins are familiar with sudo.

fabe updated this revision to Diff 26500.Dec 23 2014, 3:29 PM

add the missing setup check

Two minor inlines -- I can tweak them in the pull, just not 100% sure about the second one.

It would be nice to show the runningAsUser on the web console interface too, although we could easily deal with that later.

resources/sql/autopatches/20141223.daemonloguser.sql
3

Use VARCHAR(255) COLLATE {$COLLATE_TEXT} to make sure this picks up the correct collation.

src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
248

I'd expect cd %s && ... to be unnecessary given that setCWD() is called below. Is this just a copy/paste thing or is something more subtle going on?

(phutil_passthru() did not support setCWD() when this code was written, which is why cd %s && ... is used above, although it's technically possible to rewrite the phutil_passthru() with PhutilExcePassthru now, which does support setCWD().)

fabe added a comment.Dec 23 2014, 4:04 PM

Yeah you're right. the cd %s && is redundant. I added it when i had some problems with 'su'. But for sudo it works fine without.

fabe added a comment.Dec 23 2014, 4:05 PM

give me a moment and i'll send a new diff which fixes this and also adds the user to the web ui.

fabe updated this revision to Diff 26501.Dec 23 2014, 4:09 PM

fix inlines and add user field to webui

epriestley accepted this revision.Dec 23 2014, 4:11 PM
epriestley added a reviewer: epriestley.

Perfect, thanks! I filed T6806 as a reminder to make this more severe eventually.

This revision is now accepted and ready to land.Dec 23 2014, 4:11 PM
epriestley added inline comments.Dec 23 2014, 4:15 PM
src/applications/daemon/storage/PhabricatorDaemonLog.php
32

One tiny issue I caught in the pull -- since this is nullable (which seems reasonable, as existing rows have no meaningful value), the spec should be text255? rather than text255. bin/storage adjust will warn you about this, or it should be shown in Config -> Database Issues in the web UI.

(We'll get all this magic documented better at some point, it landed relatively-recently and wasn't very stable for a while.)

This revision was automatically updated to reflect the committed changes.