[ovs-dev,Debian-non-root,v2,4/4] Debian: start daemons as ovs(non-root) user
diff mbox

Message ID 1444436004-25557-4-git-send-email-azhou@nicira.com
State Changes Requested
Headers show

Commit Message

Andy Zhou Oct. 10, 2015, 12:13 a.m. UTC
Changes to Debian packaging scripts to create the ovs user and group.
Fix the permissions of ovs created files and directories so that
they are accessible by users belong to the ovs group.
Start daemons as the ovs user.

Signed-off-by: Andy Zhou <azhou@nicira.com>

----
This patch does not include changes to the ipsec package. Ansis has
other plans for updating it.
---
 NEWS                                       |  3 +-
 debian/automake.mk                         |  1 +
 debian/control                             |  1 +
 debian/openvswitch-common.postinst         | 45 ++++++++++++++++++++++++++++++
 debian/openvswitch-pki.postinst            |  5 ++++
 debian/openvswitch-switch.init             |  1 +
 debian/openvswitch-switch.logrotate        |  2 +-
 debian/openvswitch-switch.postinst         |  6 ++++
 debian/openvswitch-testcontroller.init     |  8 +++++-
 debian/openvswitch-testcontroller.postinst |  5 ++++
 debian/openvswitch-vtep.init               |  9 +++++-
 11 files changed, 82 insertions(+), 4 deletions(-)
 create mode 100755 debian/openvswitch-common.postinst

Comments

Ben Pfaff Oct. 24, 2015, 9:36 p.m. UTC | #1
On Fri, Oct 09, 2015 at 05:13:24PM -0700, Andy Zhou wrote:
> Changes to Debian packaging scripts to create the ovs user and group.
> Fix the permissions of ovs created files and directories so that
> they are accessible by users belong to the ovs group.
> Start daemons as the ovs user.
> 
> Signed-off-by: Andy Zhou <azhou@nicira.com>
> 
> ----
> This patch does not include changes to the ipsec package. Ansis has
> other plans for updating it.

This looks carefully done.  Thank you!  I have a few suggestions, see
below.

> +case "$1" in
> +    configure)
> +        LOGDIR=/var/log/openvswitch
> +        # Create the ovs user and group.
> +        adduser --system --group --no-create-home --quiet $OVS_USER || true

Based on looking at other packages, I'd suggest adding --disabled-login
to this command.

I am not sure why || true is there.  If adduser fails, then I suspect
that configuration should fail.  I only see || true (or similar) in a
minority of other packages that add users.

From looking at other packages, it looks like there's an unwritten
convention that a daemon's home directory should be its rundir, e.g. add
"--home /var/run/openvswitch".

A number of other packages check whether the account already exists
before it creates it.  adduser is supposed to work OK in this case, as
long as nothing needs to change, but it might be considered best
practice to check.  e.g. here is what exim4-base does:

        if ! getent passwd Debian-exim > /dev/null ; then
          echo 'Adding system-user for exim (v4)' 1>&2
          adduser --system --group --quiet --home /var/spool/exim4 \
            --no-create-home --disabled-login --force-badname Debian-exim
        fi

openvswitch-vtep.init seems like a funny place to do the following:

> +    chown -R $OVS_USER:$OVS_GROUP /etc/openvswitch
> +    chown -R $OVS_USER:$OVS_GROUP /var/run/openvswitch
> +    chmod -R 0770 /var/run/openvswitch

Also, the 770 permissions for /var/run/openvswitch mean that
unprivileged users can't see the OVS pidfiles that can reliably report
what OVS daemons are running.  Based on looking at my own system, this
is somewhat unusual (try running "find /var/run/ -maxdepth 1 -type d
-ls" and look at your results).
Andy Zhou Oct. 26, 2015, 4:36 a.m. UTC | #2
On Sat, Oct 24, 2015 at 2:36 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Fri, Oct 09, 2015 at 05:13:24PM -0700, Andy Zhou wrote:
>> Changes to Debian packaging scripts to create the ovs user and group.
>> Fix the permissions of ovs created files and directories so that
>> they are accessible by users belong to the ovs group.
>> Start daemons as the ovs user.
>>
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>>
>> ----
>> This patch does not include changes to the ipsec package. Ansis has
>> other plans for updating it.
>
> This looks carefully done.  Thank you!  I have a few suggestions, see
> below.
>
>> +case "$1" in
>> +    configure)
>> +        LOGDIR=/var/log/openvswitch
>> +        # Create the ovs user and group.
>> +        adduser --system --group --no-create-home --quiet $OVS_USER || true
>
> Based on looking at other packages, I'd suggest adding --disabled-login
> to this command.
>
> I am not sure why || true is there.  If adduser fails, then I suspect
> that configuration should fail.  I only see || true (or similar) in a
> minority of other packages that add users.
>
> From looking at other packages, it looks like there's an unwritten
> convention that a daemon's home directory should be its rundir, e.g. add
> "--home /var/run/openvswitch".
>
> A number of other packages check whether the account already exists
> before it creates it.  adduser is supposed to work OK in this case, as
> long as nothing needs to change, but it might be considered best
> practice to check.  e.g. here is what exim4-base does:
>
>         if ! getent passwd Debian-exim > /dev/null ; then
>           echo 'Adding system-user for exim (v4)' 1>&2
>           adduser --system --group --quiet --home /var/spool/exim4 \
>             --no-create-home --disabled-login --force-badname Debian-exim
>         fi
Those are good suggestions.  Will adopt all in the next rev.

>
> openvswitch-vtep.init seems like a funny place to do the following:
>
>> +    chown -R $OVS_USER:$OVS_GROUP /etc/openvswitch
>> +    chown -R $OVS_USER:$OVS_GROUP /var/run/openvswitch
>> +    chmod -R 0770 /var/run/openvswitch
>

I agree it is odd. But seems necessary given the directory and files
created above as root. Do you have
some alternatives in mind?

> Also, the 770 permissions for /var/run/openvswitch mean that
> unprivileged users can't see the OVS pidfiles that can reliably report
> what OVS daemons are running.  Based on looking at my own system, this
> is somewhat unusual (try running "find /var/run/ -maxdepth 1 -type d
> -ls" and look at your results).
I see. How about 775?
Ben Pfaff Oct. 26, 2015, 4:40 a.m. UTC | #3
On Sun, Oct 25, 2015 at 09:36:03PM -0700, Andy Zhou wrote:
> On Sat, Oct 24, 2015 at 2:36 PM, Ben Pfaff <blp@nicira.com> wrote:
> > openvswitch-vtep.init seems like a funny place to do the following:
> >
> >> +    chown -R $OVS_USER:$OVS_GROUP /etc/openvswitch
> >> +    chown -R $OVS_USER:$OVS_GROUP /var/run/openvswitch
> >> +    chmod -R 0770 /var/run/openvswitch
> >
> 
> I agree it is odd. But seems necessary given the directory and files
> created above as root. Do you have
> some alternatives in mind?

My point is that, if these files and directories need specific
permissions, then openvswitch-vtep is probably not the right place to do
it because most users won't install that package: it is only for
hardware VTEPs.  (Do you do this somewhere else too?  I only spotted it
in openvswitch-vtep.)
Andy Zhou Oct. 26, 2015, 11:20 p.m. UTC | #4
On Sun, Oct 25, 2015 at 9:40 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Sun, Oct 25, 2015 at 09:36:03PM -0700, Andy Zhou wrote:
>> On Sat, Oct 24, 2015 at 2:36 PM, Ben Pfaff <blp@nicira.com> wrote:
>> > openvswitch-vtep.init seems like a funny place to do the following:
>> >
>> >> +    chown -R $OVS_USER:$OVS_GROUP /etc/openvswitch
>> >> +    chown -R $OVS_USER:$OVS_GROUP /var/run/openvswitch
>> >> +    chmod -R 0770 /var/run/openvswitch
>> >
>>
>> I agree it is odd. But seems necessary given the directory and files
>> created above as root. Do you have
>> some alternatives in mind?
>
> My point is that, if these files and directories need specific
> permissions, then openvswitch-vtep is probably not the right place to do
> it because most users won't install that package: it is only for
> hardware VTEPs.  (Do you do this somewhere else too?  I only spotted it
> in openvswitch-vtep.)
Got it. The intention of those are to deal with VTEP dir and files
specifically.  The post install
scripts of other packages should also take care of ownership and
permission changes for those
directories -- But they are not complete in v2.  I just posted v3 that
should address those.

Patch
diff mbox

diff --git a/NEWS b/NEWS
index cdf2815..8f0e5b6 100644
--- a/NEWS
+++ b/NEWS
@@ -23,7 +23,8 @@  Post-v2.4.0
    - Dropped support for GRE64 tunnel.
    - Mark --syslog-target argument as deprecated.  It will be removed in
      the next OVS release.
-   - Added --user option to all daemons
+   - Added --user option to all daemons.
+   - Debain package starts daemons as the 'ovs' user.
 
 
 v2.4.0 - 20 Aug 2015
diff --git a/debian/automake.mk b/debian/automake.mk
index c29a560..3092569 100644
--- a/debian/automake.mk
+++ b/debian/automake.mk
@@ -8,6 +8,7 @@  EXTRA_DIST += \
 	debian/dkms.conf.in \
 	debian/dirs \
 	debian/openvswitch-common.dirs \
+	debian/openvswitch-common.postinst \
 	debian/openvswitch-common.docs \
 	debian/openvswitch-common.install \
 	debian/openvswitch-common.manpages \
diff --git a/debian/control b/debian/control
index 3eac644..7c07cb2 100644
--- a/debian/control
+++ b/debian/control
@@ -60,6 +60,7 @@  Architecture: linux-any
 Depends: openssl,
          python,
          python (>= 2.7) | python-argparse,
+         adduser,
          ${misc:Depends},
          ${shlibs:Depends}
 Suggests: ethtool
diff --git a/debian/openvswitch-common.postinst b/debian/openvswitch-common.postinst
new file mode 100755
index 0000000..cc42b13
--- /dev/null
+++ b/debian/openvswitch-common.postinst
@@ -0,0 +1,45 @@ 
+#!/bin/sh
+# postinst script for openvswitch-switch
+#
+# see: dh_installdeb(1)
+
+set -e
+
+OVS_USER=ovs
+OVS_GROUP=$OVS_USER
+
+# summary of how this script can be called:
+#        * <postinst> `configure' <most-recently-configured-version>
+#        * <old-postinst> `abort-upgrade' <new version>
+#        * <conflictor's-postinst> `abort-remove' `in-favour' <package>
+#          <new-version>
+#        * <postinst> `abort-remove'
+#        * <deconfigured's-postinst> `abort-deconfigure' `in-favour'
+#          <failed-install-package> <version> `removing'
+#          <conflicting-package> <version>
+# for details, see http://www.debian.org/doc/debian-policy/ or
+# the debian-policy package
+
+case "$1" in
+    configure)
+        LOGDIR=/var/log/openvswitch
+        # Create the ovs user and group.
+        adduser --system --group --no-create-home --quiet $OVS_USER || true
+        adduser $OVS_USER adm || true
+
+        # Fix ownership.
+        chown -R $OVS_USER:$OVS_GROUP $LOGDIR
+        ;;
+
+    abort-upgrade|abort-remove|abort-deconfigure)
+        ;;
+
+    *)
+        echo "postinst called with unknown argument \`$1'" >&2
+        exit 1
+        ;;
+esac
+
+#DEBHELPER#
+
+exit 0
diff --git a/debian/openvswitch-pki.postinst b/debian/openvswitch-pki.postinst
index f4705e9..b7821d4 100755
--- a/debian/openvswitch-pki.postinst
+++ b/debian/openvswitch-pki.postinst
@@ -5,6 +5,9 @@ 
 
 set -e
 
+OVS_USER=ovs
+OVS_GROUP=$OVS_USER
+
 # summary of how this script can be called:
 #        * <postinst> `configure' <most-recently-configured-version>
 #        * <old-postinst> `abort-upgrade' <new version>
@@ -31,6 +34,8 @@  case "$1" in
         if test ! -e /var/lib/openvswitch/pki; then
             ovs-pki init
         fi
+
+        chown -R $OVS_USER:$OVS_GROUP /var/lib/openvswitch
         ;;
 
     abort-upgrade|abort-remove|abort-deconfigure)
diff --git a/debian/openvswitch-switch.init b/debian/openvswitch-switch.init
index 8e156da..febf414 100755
--- a/debian/openvswitch-switch.init
+++ b/debian/openvswitch-switch.init
@@ -64,6 +64,7 @@  start () {
     if test X"$FORCE_COREFILES" != X; then
 	set "$@" --force-corefiles="$FORCE_COREFILES"
     fi
+    set "$@" --no-run-as-root
     set "$@" $OVS_CTL_OPTS
     "$@" || exit $?
     if [ "$2" = "start" ] && [ "$READ_INTERFACES" != "no" ]; then
diff --git a/debian/openvswitch-switch.logrotate b/debian/openvswitch-switch.logrotate
index a7a71bd..e93c568 100644
--- a/debian/openvswitch-switch.logrotate
+++ b/debian/openvswitch-switch.logrotate
@@ -1,7 +1,7 @@ 
 /var/log/openvswitch/*.log {
     daily
     compress
-    create 640 root adm
+    create 640 ovs adm
     delaycompress
     missingok
     rotate 30
diff --git a/debian/openvswitch-switch.postinst b/debian/openvswitch-switch.postinst
index 2464572..0879c7c 100755
--- a/debian/openvswitch-switch.postinst
+++ b/debian/openvswitch-switch.postinst
@@ -5,6 +5,9 @@ 
 
 set -e
 
+OVS_USER=ovs
+OVS_GROUP=$OVS_USER
+
 # summary of how this script can be called:
 #        * <postinst> `configure' <most-recently-configured-version>
 #        * <old-postinst> `abort-upgrade' <new version>
@@ -33,6 +36,9 @@  case "$1" in
                 fi
             done
 	fi
+
+	# fix owner and permissions for /etc/openvswitch.
+	chown -R $OVS_USER:$OVS_GROUP /etc/openvswitch
         ;;
 
     abort-upgrade|abort-remove|abort-deconfigure)
diff --git a/debian/openvswitch-testcontroller.init b/debian/openvswitch-testcontroller.init
index 67b7a99..d67500e 100755
--- a/debian/openvswitch-testcontroller.init
+++ b/debian/openvswitch-testcontroller.init
@@ -37,6 +37,8 @@  DAEMON=/usr/bin/ovs-testcontroller # Introduce the server's location here
 NAME=ovs-testcontroller         # Introduce the short server's name here
 DESC=ovs-testcontroller         # Introduce a short description here
 LOGDIR=/var/log/openvswitch	# Log directory to use
+OVS_USER=ovs
+OVS_GROUP=$OVS_USER
 
 PIDFILE=/var/run/openvswitch/$NAME.pid
 
@@ -109,7 +111,10 @@  start_server() {
     fi
 
     if [ ! -d /var/run/openvswitch ]; then
-        install -d -m 755 -o root -g root /var/run/openvswitch
+        install -d -m 770 -o $OVS_USER -g $OVS_GROUP /var/run/openvswitch
+    else
+        chown -R $OVS_USER:$OVS_GROUP /var/run/openvswitch
+        chmod 0770 -R /var/run/openvswitch
     fi
 
     SSL_OPTS=
@@ -139,6 +144,7 @@  start_server() {
         if [ -z "$DAEMONUSER" ] ; then
             start-stop-daemon --start --pidfile $PIDFILE \
                         --exec $DAEMON -- --detach --pidfile=$PIDFILE \
+                        --user $OVS_USER:$OVS_GROUP \
                         $LISTEN $DAEMON_OPTS $SSL_OPTS
             errcode=$?
         else
diff --git a/debian/openvswitch-testcontroller.postinst b/debian/openvswitch-testcontroller.postinst
index 7242b4a..ee7f4c7 100755
--- a/debian/openvswitch-testcontroller.postinst
+++ b/debian/openvswitch-testcontroller.postinst
@@ -5,6 +5,9 @@ 
 
 set -e
 
+OVS_USER=ovs
+OVS_GROUP=$OVS_USER
+
 # summary of how this script can be called:
 #        * <postinst> `configure' <most-recently-configured-version>
 #        * <old-postinst> `abort-upgrade' <new version>
@@ -42,6 +45,8 @@  case "$1" in
             chmod go+r cert.pem req.pem
             umask $oldumask
         fi
+
+        chown -R $OVS_USER:$OVS_GROUP /etc/openvswitch-testcontroller
         ;;
 
     abort-upgrade|abort-remove|abort-deconfigure)
diff --git a/debian/openvswitch-vtep.init b/debian/openvswitch-vtep.init
index ebf4e26..05a0a68 100644
--- a/debian/openvswitch-vtep.init
+++ b/debian/openvswitch-vtep.init
@@ -10,6 +10,8 @@ 
 # Description:       Initializes the Open vSwitch VTEP emulator
 ### END INIT INFO
 
+OVS_USER=ovs
+OVS_GROUP=$OVS_USER
 
 # Include defaults if available
 default=/etc/default/openvswitch-vtep
@@ -40,17 +42,22 @@  start () {
         cd /etc/openvswitch && ovs-pki req ovsclient && ovs-pki self-sign ovsclient
     fi
 
+    chown -R $OVS_USER:$OVS_GROUP /etc/openvswitch
+    chown -R $OVS_USER:$OVS_GROUP /var/run/openvswitch
+    chmod -R 0770 /var/run/openvswitch
+
     ovsdb-server --pidfile --detach --log-file --remote \
         punix:/var/run/openvswitch/db.sock \
         --remote=db:hardware_vtep,Global,managers \
         --private-key=/etc/openvswitch/ovsclient-privkey.pem \
         --certificate=/etc/openvswitch/ovsclient-cert.pem \
         --bootstrap-ca-cert=/etc/openvswitch/vswitchd.cacert \
+        --user $OVS_USER:$OVS_GROUP \
         /etc/openvswitch/conf.db /etc/openvswitch/vtep.db
 
     modprobe openvswitch
 
-    ovs-vswitchd --pidfile --detach --log-file \
+    ovs-vswitchd --pidfile --detach --log-file --user $OVS_USER:$OVS_GROUP \
         unix:/var/run/openvswitch/db.sock
 }