| Message ID | 1444436004-25557-4-git-send-email-azhou@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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).
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?
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.)
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.
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 }
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