[arch-projects] [initscripts][PATCH 2/2] rc.d: Add check to cleanly abort rc.d script if user doesn't have root privileges

[Eric Bélanger]

On Mon, May 30, 2011 at 6:23 PM, Seblu <seblu at seblu.net> wrote:
> On Sat, May 28, 2011 at 5:52 AM, Eric Bélanger <snowmaniscool at gmail.com> wrote:
>> On Fri, May 27, 2011 at 10:19 AM, Seblu <seblu at seblu.net> wrote:
>>> On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger <snowmaniscool at gmail.com> wrote:
>>>> This implements FS#24095. The check is only made for the start, stop and restart
>>>> actions of the daemon scripts. This allows regular user to use the help and list
>>>> functionality of rc.d and also to use rc.d for actions that doesn't require root
>>>> privileges, like the status action of some daemon scripts.
>>>>
>>>> Signed-off-by: Eric Bélanger <snowmaniscool at gmail.com>
>>>> ---
>>>>  rc.d |    4 ++++
>>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/rc.d b/rc.d
>>>> index 97f266a..2325623 100755
>>>> --- a/rc.d
>>>> +++ b/rc.d
>>>> @@ -43,6 +43,10 @@ case $1 in
>>>>                ;;
>>>>        *)
>>>>                action=$1
>>>> +               if [[ "$EUID" != '0' ]] && [[ "$action" == 'start' || "$action" == 'stop' || "$action" == 'restart' ]] ; then
>>>> +                   echo 'Error: this script must be run as root to use this functionality.'
>>>> +                   exit 1
>>>> +               fi
>>>>                shift
>>>>                # set same environment variables as init
>>>>                runlevel=$(/sbin/runlevel)
>>>
>>> As i said in FS#24095, if we really want do this, we should not do
>>> this in rc.d script but in functions which is loaded by real rc
>>> scripts.
>>>
>>> Increasingly, why choose start/stop/restart and not reload by example?
>>> By example, in virtualbox_bin we have fixusb, which must be run as root.
>>>
>>
>> I only chose start/stop/restart because they are the only standard
>> ones.
> There is a page describing arch standard, or at least, a namming convention?

The only one I can think of is the daemon prototype file which has a
status function to display if the daemon is running or not. There are
2 packages that I maintain (or have maintained) that use the status
for something else so it seem to be more standard than I thought. The
reload action seem to be quite common and standard. Later tonight,
I'll go through all the daemons in the repos and list what action they
have (except start/stop/restart) and what these actions do.  It'll
give use a better idea if there is a genereal concensus on these
various actions.

>
>> Not only all daemons script have them but they all do the same
>> things: start/stop a binary before creating/removing a file
>> contatining the PID in a directory that needs root privileges.  I am
>> 100% sure that root privilege is required.  The other actions are not
>> used by all daemons and what they do depends on the daemon itself. So
>> we can't be really sure if root privileges are required or not.
>>
>> I decided to play it safe and to treat everything else than
>> start/stop/restart as edge cases.
> It makes sense, but I am forbidden to do
>
>>> I think we should offer a check_root function which can be called in
>>> rc scripts to ensure rootitude. Be we cannot generically know if a rc
>>> need to be root or not.
>>
>> That might be ideal.  But for the reason I mentionned above, the
>> daemon scripts will probably need to be modified to indicate what
>> privileges are needed for all actions.  Perhaps, demanding root
>> privileges by default except when there is a NEED_ROOT=0 defined in
>> the case, e.g.
> i agree. Idea is better.
>
>>
>>  status)
>>     NEED_ROOT=0
>>     do stuff that don't need root privs...
> Don't easy to implement in bash...
>
>> would reduce the workload on fixing the packages. That will surely
>> take some time before it's implemented unless that can be done without
>> modifying the daemon scripts.
> As said before, we can use functions scripts which is sourced by all
> rc.d scripts to make this check rather than doing it in rc.d
>
>>
>> Until this is done, this proposed patch is a good compromise between
>> the ideal situation and the current situation where no check is done
>> at all.
> Basically, there is maybe no problem to don't check. This is not done
> from the beginning.
> If you can, you can. If you need right it will fail. KISS.
>
>> The majority of users will use rc.d to start/stop/restart
>> daemons anyway.  Since you can use rc.d on many daemons at once, it's
>> better to abort the rc.d script early so you can rerun it with
>> sufficient privileges instead of having to wait for all the daemons to
>> fail.  It would also be trivial to remove this check in rc.d once (or
>> if) we have implemented something better.
>>
> I do not see the difference. If all daemons will need root privileges,
> they will fail.
>
> I think a lots of users will still use /etc/rc.d/x start/stop/restart,
> and it's kind of sugar around if you can or not run a daemon.
>
> I propose a patch following your recommendation here :
> https://github.com/seblu/arch-initscripts/commit/5a59a30
>
> What do you think?

Your patch doesn't do what I was thinking mainly because my idea
wasn't a good one.  The problem is that the checking is done in
/etc/rc.d/functions which is sourced at the begining of the script so
the NEED_ROOT variable needs to be defined globally while I wanted a
local approach that would depend on the items in the case statement.
I don't know if it's clear but it doesn't matter because I found a
better solution that works without needing the NEED_ROOT  variable.

We need to add in /etc/rc.d/functions your need_root function followed by:

if [[ "$1" == 'start' || "$1" == 'stop' || "$1" == 'restart' ]]; then
        need_root
fi

I only have start/stop/restart in that code sample but we'll probably
need to add more actions , like reload, depending on what I find in
the daemon investigation I described above.  I did tests with deamon
scripts and rc.d and it works without any modifications needed to
them.

Couple drawbacks:

- The list of actions to test for root is hard-coded. If I add a
foobar action which requires root privileges to a daemon, it'll need
to be added to the list in /etc/rc.d/functions. As this should happen
often, it would be minimal maintenance.

- It suppose that all daemon using a given action need the same
privileges. If it's not the case, we'll need to either fix the
packages, enforce root privileges or go with the majority depnding on
the case.

- When thinking about this, I only had the rc.d and daemon scripts in
mind and I monly testes these. If there are other scripts which
sources /etc/rc.d/functions, we'll need to check them to see if they
still work fine with my change.

>
>
> --
> Sébastien Luttringer
> www.seblu.net
>