[arch-projects] [initscripts][PATCH 2/2] rc.d: Add check to cleanly abort rc.d script if user doesn't have root privileges
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@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)
On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger snowmaniscool@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@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 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.
Regards,
On Fri, May 27, 2011 at 10:19 AM, Seblu seblu@seblu.net wrote:
On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger snowmaniscool@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@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. 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. I assumed that the use know what privileges they require and will run rc.d with the needed privileges. If they don't, they'll get the FAIL message from the daemon script.
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.
status) NEED_ROOT=0 do stuff that don't need root privs... ;;
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.
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. 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.
Eric
Regards,
-- Sébastien Luttringer www.seblu.net
On Sat, May 28, 2011 at 5:52 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Fri, May 27, 2011 at 10:19 AM, Seblu seblu@seblu.net wrote:
On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger snowmaniscool@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@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?
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?
On Mon, May 30, 2011 at 6:23 PM, Seblu seblu@seblu.net wrote:
On Sat, May 28, 2011 at 5:52 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Fri, May 27, 2011 at 10:19 AM, Seblu seblu@seblu.net wrote:
On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger snowmaniscool@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@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
On Tue, May 31, 2011 at 7:29 PM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Mon, May 30, 2011 at 6:23 PM, Seblu seblu@seblu.net wrote:
On Sat, May 28, 2011 at 5:52 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Fri, May 27, 2011 at 10:19 AM, Seblu seblu@seblu.net wrote:
On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger snowmaniscool@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@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.
Hi,
Here's my findings. Basically, all daemons actions need root privileges except a few of them:
- iflist) and rtlist) from network daemon - reload) action for the dbus daemon. As all other daemon require root privileges for reload, it will be best to enforce it for the reload action. - status). A majority of daemon don't need root privilege for it but, again, there are exceptions: - ufw: looks like a bug in its daemon file. I'll submit a bug report. - laptop-mode-tools: With user privs, you get some information about the system except the ones that need direct access to the hard drive devices. Not really worth fixing. Let's ignore it. - apcupsd: I can't test this one as I don't have a UPS.I get the same error message whether I have root privileges or not so it might not require them.
Long story short: I suggest enforcing root privileges for all daemon actions except: iflist, rtlist and status. So we have:
need_root() { (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1 }
if [[ "$1" == 'attach' || "$1" == 'careless_start' || "$1" == 'check' || \ "$1" == 'clean' || "$1" == 'condrestart' || "$1" == 'down' || \ "$1" == 'force-quit' || "$1" == 'force-reload' || "$1" == 'force-restart' || \ "$1" == 'ifdown' || "$1" == 'ifup' || "$1" == 'load' || \ "$1" == 'logrotate' || "$1" == 'purge' || "$1" == 'reload' || \ "$1" == 'restart' || "$1" == 'resume' || "$1" == 'rtdown' || "$1" == 'rtup' || \ "$1" == 'save' || "$1" == 'setup' || "$1" == 'start' || "$1" == 'stop' || \ "$1" == 'suspend' || "$1" == 'unload' || "$1" == 'up' ]]; then need_root fi
Eric
-- Sébastien Luttringer www.seblu.net
On Thu, Jun 2, 2011 at 2:30 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Tue, May 31, 2011 at 7:29 PM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Mon, May 30, 2011 at 6:23 PM, Seblu seblu@seblu.net wrote:
On Sat, May 28, 2011 at 5:52 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Fri, May 27, 2011 at 10:19 AM, Seblu seblu@seblu.net wrote:
On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger snowmaniscool@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@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.
Hi,
Here's my findings. Basically, all daemons actions need root privileges except a few of them:
- iflist) and rtlist) from network daemon
- reload) action for the dbus daemon. As all other daemon require
root privileges for reload, it will be best to enforce it for the reload action.
- status). A majority of daemon don't need root privilege for it but,
again, there are exceptions: - ufw: looks like a bug in its daemon file. I'll submit a bug report. - laptop-mode-tools: With user privs, you get some information about the system except the ones that need direct access to the hard drive devices. Not really worth fixing. Let's ignore it. - apcupsd: I can't test this one as I don't have a UPS.I get the same error message whether I have root privileges or not so it might not require them.
Long story short: I suggest enforcing root privileges for all daemon actions except: iflist, rtlist and status. So we have:
need_root() { (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1 }
if [[ "$1" == 'attach' || "$1" == 'careless_start' || "$1" == 'check' || \ "$1" == 'clean' || "$1" == 'condrestart' || "$1" == 'down' || \ "$1" == 'force-quit' || "$1" == 'force-reload' || "$1" == 'force-restart' || \ "$1" == 'ifdown' || "$1" == 'ifup' || "$1" == 'load' || \ "$1" == 'logrotate' || "$1" == 'purge' || "$1" == 'reload' || \ "$1" == 'restart' || "$1" == 'resume' || "$1" == 'rtdown' || "$1" == 'rtup' || \ "$1" == 'save' || "$1" == 'setup' || "$1" == 'start' || "$1" == 'stop' || \ "$1" == 'suspend' || "$1" == 'unload' || "$1" == 'up' ]]; then need_root fi
Hi,
Thanks for crawling initscripts to find problematic options. You probably miss some case in aur, where rc.d scripts implement "strange" options which need root priv, for example virtualbox_bin which have a fixsub.
I really don't like this idea of hardcode which options should or should not needs root priv in initscripts, because, I think we should let rc.d scripts depends of initscripts package, and not initscripts package depends of ufw, laptop-tools or others.
I also think we should be able to resume by a clear and simple policy like : By default initscripts need to be root (Thomas in FS#24095). If you want bypass this assumption to offer a by options (or by what you want) right management, just disable and do what you want. need_root is just a proposed generic helper to enforce root policy. But, scripts maintainer can check if user who ask something is member of a group to allow his script.
Intelligence is in the script, we should not try to predict what it will do a in special cases (options). Let script maintainers choose.
Cheers,
On Fri, Jun 17, 2011 at 7:25 PM, Seblu seblu@seblu.net wrote:
On Thu, Jun 2, 2011 at 2:30 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Tue, May 31, 2011 at 7:29 PM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Mon, May 30, 2011 at 6:23 PM, Seblu seblu@seblu.net wrote:
On Sat, May 28, 2011 at 5:52 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
On Fri, May 27, 2011 at 10:19 AM, Seblu seblu@seblu.net wrote:
On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger snowmaniscool@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@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.
Hi,
Here's my findings. Basically, all daemons actions need root privileges except a few of them:
- iflist) and rtlist) from network daemon
- reload) action for the dbus daemon. As all other daemon require
root privileges for reload, it will be best to enforce it for the reload action.
- status). A majority of daemon don't need root privilege for it but,
again, there are exceptions: - ufw: looks like a bug in its daemon file. I'll submit a bug report. - laptop-mode-tools: With user privs, you get some information about the system except the ones that need direct access to the hard drive devices. Not really worth fixing. Let's ignore it. - apcupsd: I can't test this one as I don't have a UPS.I get the same error message whether I have root privileges or not so it might not require them.
Long story short: I suggest enforcing root privileges for all daemon actions except: iflist, rtlist and status. So we have:
need_root() { (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1 }
if [[ "$1" == 'attach' || "$1" == 'careless_start' || "$1" == 'check' || \ "$1" == 'clean' || "$1" == 'condrestart' || "$1" == 'down' || \ "$1" == 'force-quit' || "$1" == 'force-reload' || "$1" == 'force-restart' || \ "$1" == 'ifdown' || "$1" == 'ifup' || "$1" == 'load' || \ "$1" == 'logrotate' || "$1" == 'purge' || "$1" == 'reload' || \ "$1" == 'restart' || "$1" == 'resume' || "$1" == 'rtdown' || "$1" == 'rtup' || \ "$1" == 'save' || "$1" == 'setup' || "$1" == 'start' || "$1" == 'stop' || \ "$1" == 'suspend' || "$1" == 'unload' || "$1" == 'up' ]]; then need_root fi
Hi,
Thanks for crawling initscripts to find problematic options. You probably miss some case in aur, where rc.d scripts implement "strange" options which need root priv, for example virtualbox_bin which have a fixsub.
When you mentionned virtualbox_bin in an earlier post, I thought it was one of the virtualbox packages in community but couldn't find it when I went trough the daemon scripts. FTR, I didn't check the AUR, the repos were enough work. ;)
I really don't like this idea of hardcode which options should or should not needs root priv in initscripts, because, I think we should let rc.d scripts depends of initscripts package, and not initscripts package depends of ufw, laptop-tools or others.
Yeah, hardcoding is not very practical. That's why I initially wanted to have the permission info in the individual daemon file but that doesn't work.
I also think we should be able to resume by a clear and simple policy like : By default initscripts need to be root (Thomas in FS#24095). If you want bypass this assumption to offer a by options (or by what you want) right management, just disable and do what you want. need_root is just a proposed generic helper to enforce root policy. But, scripts maintainer can check if user who ask something is member of a group to allow his script.
Sure, checking for root privileges for all actions is the easiest and simplest method. However, the status actions which currently, in most cases, can done as a regular user will require root privileges from now on if we go this way.
I have another (better, IMO) idea: enforce root privilege for all actions except status. Pros: - no hardcoded list - regular users will still need be able to do: /etc/rc.d/foo status
For the few packages that currently require root privilege for the status action, we can argue that they are misusing the status function as they don't follow the daemon prototype /usr/share/pacman/rc-script.proto which has: status) stat_busy "Checking $daemon_name status"; ck_status $daemon_name ;; which doesn't need root privilege. So their current status function should be renamed to info, for example.
This would imply simply adding the following to /etc/rc.d/functions :
need_root() { (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1 }
if [[ "$1" != 'status' ]]; then need_root fi
Eric
Intelligence is in the script, we should not try to predict what it will do a in special cases (options). Let script maintainers choose.
Cheers,
-- Sébastien Luttringer www.seblu.net
On Sat, Jun 18, 2011 at 5:34 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
Sure, checking for root privileges for all actions is the easiest and simplest method. However, the status actions which currently, in most cases, can done as a regular user will require root privileges from now on if we go this way.
Yes. We speak of about 23 packages where a rc.d script should be corrected about status.
cd /var/abs && grep -ril '/etc/rc.d/functions' *|xargs grep --color 'status)'|wc -l
I'm ready to propose an updated version of this 23 rc.d script, if there is a workload isssue.
I have another (better, IMO) idea: enforce root privilege for all actions except status. Pros:
- no hardcoded list
you can see status as a list of 1 element ;)
- regular users will still need be able to do: /etc/rc.d/foo status
For the few packages that currently require root privilege for the status action, we can argue that they are misusing the status function as they don't follow the daemon prototype /usr/share/pacman/rc-script.proto which has: status) stat_busy "Checking $daemon_name status"; ck_status $daemon_name ;; which doesn't need root privilege. So their current status function should be renamed to info, for example.
This would imply simply adding the following to /etc/rc.d/functions :
need_root() { (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1 }
if [[ "$1" != 'status' ]]; then need_root fi
Doesn't forget, /etc/rc.d/functions are sourced by others script than rc.d scripts. For example /var/abs/extra/ifplugd/ifplugd.action which doesn't have $1 the same meaning as your code expect.
Even if your solution is a compromise, I think in the case of status (23 pkg in the official repo), there is no need to make an exception to do things well.
Tom / Dave / Thomas have you an opinion?
On Sat, Jun 18, 2011 at 5:30 PM, Seblu seblu@seblu.net wrote:
On Sat, Jun 18, 2011 at 5:34 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
Sure, checking for root privileges for all actions is the easiest and simplest method. However, the status actions which currently, in most cases, can done as a regular user will require root privileges from now on if we go this way.
Yes. We speak of about 23 packages where a rc.d script should be corrected about status.
cd /var/abs && grep -ril '/etc/rc.d/functions' *|xargs grep --color 'status)'|wc -l
I'm ready to propose an updated version of this 23 rc.d script, if there is a workload isssue.
I have another (better, IMO) idea: enforce root privilege for all actions except status. Pros:
- no hardcoded list
you can see status as a list of 1 element ;)
- regular users will still need be able to do: /etc/rc.d/foo status
For the few packages that currently require root privilege for the status action, we can argue that they are misusing the status function as they don't follow the daemon prototype /usr/share/pacman/rc-script.proto which has: status) stat_busy "Checking $daemon_name status"; ck_status $daemon_name ;; which doesn't need root privilege. So their current status function should be renamed to info, for example.
This would imply simply adding the following to /etc/rc.d/functions :
need_root() { (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1 }
if [[ "$1" != 'status' ]]; then need_root fi
Doesn't forget, /etc/rc.d/functions are sourced by others script than rc.d scripts. For example /var/abs/extra/ifplugd/ifplugd.action which doesn't have $1 the same meaning as your code expect.
Even if your solution is a compromise, I think in the case of status (23 pkg in the official repo), there is no need to make an exception to do things well.
Tom / Dave / Thomas have you an opinion?
Dave, Tom, i see your comments in this bug : https://bugs.archlinux.org/task/25271 and this doesn't make be happy.
Here I wanted to make adjustments while maintaining the will to implement this bug. As i said from the begining, maybe we cannot want to do that...
On Thu, Jul 28, 2011 at 6:14 PM, Seblu seblu@seblu.net wrote:
On Sat, Jun 18, 2011 at 5:30 PM, Seblu seblu@seblu.net wrote:
On Sat, Jun 18, 2011 at 5:34 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
Sure, checking for root privileges for all actions is the easiest and simplest method. However, the status actions which currently, in most cases, can done as a regular user will require root privileges from now on if we go this way.
Yes. We speak of about 23 packages where a rc.d script should be corrected about status.
cd /var/abs && grep -ril '/etc/rc.d/functions' *|xargs grep --color 'status)'|wc -l
I'm ready to propose an updated version of this 23 rc.d script, if there is a workload isssue.
I have another (better, IMO) idea: enforce root privilege for all actions except status. Pros:
- no hardcoded list
you can see status as a list of 1 element ;)
- regular users will still need be able to do: /etc/rc.d/foo status
For the few packages that currently require root privilege for the status action, we can argue that they are misusing the status function as they don't follow the daemon prototype /usr/share/pacman/rc-script.proto which has: status) stat_busy "Checking $daemon_name status"; ck_status $daemon_name ;; which doesn't need root privilege. So their current status function should be renamed to info, for example.
This would imply simply adding the following to /etc/rc.d/functions :
need_root() { (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1 }
if [[ "$1" != 'status' ]]; then need_root fi
Doesn't forget, /etc/rc.d/functions are sourced by others script than rc.d scripts. For example /var/abs/extra/ifplugd/ifplugd.action which doesn't have $1 the same meaning as your code expect.
Even if your solution is a compromise, I think in the case of status (23 pkg in the official repo), there is no need to make an exception to do things well.
Tom / Dave / Thomas have you an opinion?
Dave, Tom, i see your comments in this bug : https://bugs.archlinux.org/task/25271 and this doesn't make be happy.
Here I wanted to make adjustments while maintaining the will to implement this bug. As i said from the begining, maybe we cannot want to do that...
There is making adjustments, and there are regressions. Regressions are unacceptable, especially in core software like init and daemon scripts as well as network configuration.
Everyone gets the "why" here. We're just not satisfied with the implementation, or "how", as it broke things and is done in a file that should be source-able without side effects.
-Dan
On Fri, Jul 29, 2011 at 1:26 AM, Dan McGee dpmcgee@gmail.com wrote:
On Thu, Jul 28, 2011 at 6:14 PM, Seblu seblu@seblu.net wrote:
On Sat, Jun 18, 2011 at 5:30 PM, Seblu seblu@seblu.net wrote:
On Sat, Jun 18, 2011 at 5:34 AM, Eric Bélanger snowmaniscool@gmail.com wrote:
Sure, checking for root privileges for all actions is the easiest and simplest method. However, the status actions which currently, in most cases, can done as a regular user will require root privileges from now on if we go this way.
Yes. We speak of about 23 packages where a rc.d script should be corrected about status.
cd /var/abs && grep -ril '/etc/rc.d/functions' *|xargs grep --color 'status)'|wc -l
I'm ready to propose an updated version of this 23 rc.d script, if there is a workload isssue.
I have another (better, IMO) idea: enforce root privilege for all actions except status. Pros:
- no hardcoded list
you can see status as a list of 1 element ;)
- regular users will still need be able to do: /etc/rc.d/foo status
For the few packages that currently require root privilege for the status action, we can argue that they are misusing the status function as they don't follow the daemon prototype /usr/share/pacman/rc-script.proto which has: status) stat_busy "Checking $daemon_name status"; ck_status $daemon_name ;; which doesn't need root privilege. So their current status function should be renamed to info, for example.
This would imply simply adding the following to /etc/rc.d/functions :
need_root() { (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1 }
if [[ "$1" != 'status' ]]; then need_root fi
Doesn't forget, /etc/rc.d/functions are sourced by others script than rc.d scripts. For example /var/abs/extra/ifplugd/ifplugd.action which doesn't have $1 the same meaning as your code expect.
Even if your solution is a compromise, I think in the case of status (23 pkg in the official repo), there is no need to make an exception to do things well.
Tom / Dave / Thomas have you an opinion?
Dave, Tom, i see your comments in this bug : https://bugs.archlinux.org/task/25271 and this doesn't make be happy.
Here I wanted to make adjustments while maintaining the will to implement this bug. As i said from the begining, maybe we cannot want to do that...
There is making adjustments, and there are regressions. Regressions are unacceptable, especially in core software like init and daemon scripts as well as network configuration.
I agree, release with a regression is unacceptable.
To don't have regressions during introduction of checking rootiness in rc.d scripts we shoud have: 1) check that everyone wants this feature and this implementation 2) introduction NEED_ROOT=0 and need_root function in a core release 3) ask to maintainer of rc.d scripts which want to use as non root to update 5) set NEED_ROOT=1 in a testing release. 6) fix scripts we are still broken
Everyone gets the "why" here. We're just not satisfied with the implementation, or "how", as it broke things and is done in a file that should be source-able without side effects.
/etc/rc.d/functions is not a function which can be sourcable without side effects by non rc.d scripts because: - it's not a config files (so they can execute things) - PATH is changed (your program path will change) - LOCALES are reseted (your program locales will be in locale C) - checks are done on first arg
so i can resume by : it's expected to be sourced by rc.d scripts _only_.
That's why i think, if we choose to implement this kind of restriction, functions is a good place.
But netcfg, seems to used this file (i missed it).
From a little grep on my station only netcfg source it as a program .
On Fri, Jul 29, 2011 at 1:14 AM, Seblu seblu@seblu.net wrote:
Dave, Tom, i see your comments in this bug : https://bugs.archlinux.org/task/25271 and this doesn't make be happy.
Here I wanted to make adjustments while maintaining the will to implement this bug. As i said from the begining, maybe we cannot want to do that...
I just added a quick fix for this release (essentially a revert), we can figure out what to do properly for the next release.
I think we'll have to turn the logic on its head. By default we should only block things we know are always ok to block (like Eric's original patch, we could maintain a list of actions that always need root), or things which the rc script author has checked are ok to block (the inverse of how it is now, it should be possible to opt-in for requiring root).
This way we cannot get regressions in user-created scripts or scripts that just happen not to have been updated.
Remember that a false positive (trying to do something that requires root) is always fine, as the call will fail. However, a false negative (blocking something which does not need root) is not ok as it is essentially a regression in functionality as Dan points out.
Cheers,
Tom
On Fri, Jul 29, 2011 at 1:32 AM, Tom Gundersen teg@jklm.no wrote:
On Fri, Jul 29, 2011 at 1:14 AM, Seblu seblu@seblu.net wrote:
Dave, Tom, i see your comments in this bug : https://bugs.archlinux.org/task/25271 and this doesn't make be happy.
Here I wanted to make adjustments while maintaining the will to implement this bug. As i said from the begining, maybe we cannot want to do that...
I just added a quick fix for this release (essentially a revert), we can figure out what to do properly for the next release.
I think we'll have to turn the logic on its head. By default we should only block things we know are always ok to block (like Eric's original patch, we could maintain a list of actions that always need root), or things which the rc script author has checked are ok to block (the inverse of how it is now, it should be possible to opt-in for requiring root).
This way we cannot get regressions in user-created scripts or scripts that just happen not to have been updated.
It would be better not to check at all than to do a check that is useless in most cases. If rc.d maintainers wants to check rootitude, he can already do it. He don't need a patch in initscripts.
An why check rootitude, some user can have capatiblities to run it without be the root.
Remember that a false positive (trying to do something that requires root) is always fine, as the call will fail. However, a false negative (blocking something which does not need root) is not ok as it is essentially a regression in functionality as Dan points out.
That's a wise consideration. But regression is a word we give to a modification which go in a bad direction.
By example, verbosity of kernel, complex network setups in initscripts was removed. This is also regression. But we accept it because we choose it.
If we choose that initscripts must be run by root (this was the request) exept if it claim the opposit and we let times to have all scripts ready for this. This is still a regression for you?
participants (4)
-
Dan McGee
-
Eric Bélanger
-
Seblu
-
Tom Gundersen