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?

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

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

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.

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.

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