| Message ID | 1444095524-11357-4-git-send-email-azhou@nicira.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Oct 5, 2015 at 6:38 PM, Andy Zhou <azhou@nicira.com> wrote: Thanks Andy for doing this! I will have another more careful look at this patch tomorrow, because I think I somehow managed to get into a state where after installing debian packages /etc/openvswitch still belonged to root. > 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. s/users belong/users that belong > Start daemons as the ovs user. > > Signed-off-by: Andy Zhou <azhou@nicira.com> This patch, I believe, breaks upgrades: Wed Oct 7 23:35:44 UTC 2015:stop * ovs-vswitchd is not running * ovsdb-server is not running Wed Oct 7 23:35:44 UTC 2015:load-kmod Wed Oct 7 23:35:44 UTC 2015:start --system-id=random --no-run-as-root ovsdb-server: /var/run/openvswitch/ovsdb-server.pid.tmp: create failed (Permission denied) I guess this was happening because that directory still belonged to the root user after the upgrade. > > ---- > This patch does not include changes to the ipsec package. Ansis has > other plans for updating it. Yeah, I will have to figure out how to do this from Python daemons. I guess we have to synchronize our changes so that we don't break IPsec. > --- > NEWS | 3 ++- > debian/automake.mk | 1 + > debian/openvswitch-common.postinst | 42 ++++++++++++++++++++++++++++++ > debian/openvswitch-pki.postinst | 2 ++ > debian/openvswitch-switch.init | 1 + > debian/openvswitch-switch.logrotate | 2 +- > debian/openvswitch-switch.postinst | 3 +++ > debian/openvswitch-testcontroller.init | 3 ++- > debian/openvswitch-testcontroller.postinst | 2 ++ > debian/openvswitch-vtep.init | 8 +++++- > 10 files changed, 63 insertions(+), 4 deletions(-) > create mode 100755 debian/openvswitch-common.postinst > > 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. s/Debain/Debian > > > 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/openvswitch-common.postinst b/debian/openvswitch-common.postinst > new file mode 100755 > index 0000000..c90ab5a > --- /dev/null > +++ b/debian/openvswitch-common.postinst > @@ -0,0 +1,42 @@ > +#!/bin/sh > +# postinst script for openvswitch-switch Copy-paste error: This is openvswitch-common and not openvswitch-switch postinst script > +# > +# see: dh_installdeb(1) > + > +set -e > + > +# 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 || true There are useradd and adduser utilities. I tried *bare minimum* Debian 8 installation and adduser was not installed by default. Should you add adduser to dependencies in debian/control file? > + > + # Fix ownership and permissions. > + chown -R ovs:ovs $LOGDIR > + chmod -R 0770 $LOGDIR You have probably thought more about this, but now "adm" group is dropped for OVS logs. Do you see any issue with this? > + ;; > + > + 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..030180d 100755 > --- a/debian/openvswitch-pki.postinst > +++ b/debian/openvswitch-pki.postinst > @@ -31,6 +31,8 @@ case "$1" in > if test ! -e /var/lib/openvswitch/pki; then > ovs-pki init > fi > + > + chown ovs:ovs -R /var/lib/openvswitch/pki Shouldn't changing user recursively for /var/lib/openvswitch be a better approach? > ;; > > 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..be929b6 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 ovs > delaycompress > missingok > rotate 30 > diff --git a/debian/openvswitch-switch.postinst b/debian/openvswitch-switch.postinst > index 2464572..9183bdc 100755 > --- a/debian/openvswitch-switch.postinst > +++ b/debian/openvswitch-switch.postinst > @@ -33,6 +33,9 @@ case "$1" in > fi > done > fi > + > + # fix owner and permissions for /etc/openvswitch. > + chown ovs:ovs -R /etc/openvswitch can you assume that this directory unconditionally exists (I believe all debian scripts are run with set -e and you don't want them to terminate prematurely)? > ;; > > abort-upgrade|abort-remove|abort-deconfigure) > diff --git a/debian/openvswitch-testcontroller.init b/debian/openvswitch-testcontroller.init > index 67b7a99..352c95d 100755 > --- a/debian/openvswitch-testcontroller.init > +++ b/debian/openvswitch-testcontroller.init > @@ -109,7 +109,7 @@ start_server() { > fi > > if [ ! -d /var/run/openvswitch ]; then > - install -d -m 755 -o root -g root /var/run/openvswitch > + install -d -m 755 -o ovs -g ovs /var/run/openvswitch if directory exists this will not change ownership, right? > fi > > SSL_OPTS= > @@ -139,6 +139,7 @@ start_server() { > if [ -z "$DAEMONUSER" ] ; then > start-stop-daemon --start --pidfile $PIDFILE \ > --exec $DAEMON -- --detach --pidfile=$PIDFILE \ > + --user ovs:ovs \ it seems inconsistent that in some places you use --user ovs:ovs but in other --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..e8584e2 100755 > --- a/debian/openvswitch-testcontroller.postinst > +++ b/debian/openvswitch-testcontroller.postinst > @@ -42,6 +42,8 @@ case "$1" in > chmod go+r cert.pem req.pem > umask $oldumask > fi > + > + chown ovs:ovs -R /etc/openvswitch-testcontroller > ;; > > abort-upgrade|abort-remove|abort-deconfigure) > diff --git a/debian/openvswitch-vtep.init b/debian/openvswitch-vtep.init > index ebf4e26..6fe02a1 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 > > # Include defaults if available > default=/etc/default/openvswitch-vtep > @@ -40,17 +42,21 @@ 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 > + > 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 > } > > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Wed, Oct 7, 2015 at 6:49 PM, Ansis Atteka <aatteka@nicira.com> wrote: > On Mon, Oct 5, 2015 at 6:38 PM, Andy Zhou <azhou@nicira.com> wrote: > > Thanks Andy for doing this! I will have another more careful look at > this patch tomorrow, because I think I somehow managed to get into a > state where after installing debian packages /etc/openvswitch still > belonged to root. > Is possible you have your selinux changes already applied? It worked in my set up. > >> 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. > > s/users belong/users that belong Thanks, will fix. > >> Start daemons as the ovs user. >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> > > This patch, I believe, breaks upgrades: > > Wed Oct 7 23:35:44 UTC 2015:stop > * ovs-vswitchd is not running > * ovsdb-server is not running > Wed Oct 7 23:35:44 UTC 2015:load-kmod > Wed Oct 7 23:35:44 UTC 2015:start --system-id=random --no-run-as-root > ovsdb-server: /var/run/openvswitch/ovsdb-server.pid.tmp: create failed > (Permission denied) > > I guess this was happening because that directory still belonged to > the root user after the upgrade. > The error mentioned above will cause this. > >> >> ---- >> This patch does not include changes to the ipsec package. Ansis has >> other plans for updating it. > > Yeah, I will have to figure out how to do this from Python daemons. I > guess we have to synchronize our changes so that we don't break IPsec. > >> --- >> NEWS | 3 ++- >> debian/automake.mk | 1 + >> debian/openvswitch-common.postinst | 42 ++++++++++++++++++++++++++++++ >> debian/openvswitch-pki.postinst | 2 ++ >> debian/openvswitch-switch.init | 1 + >> debian/openvswitch-switch.logrotate | 2 +- >> debian/openvswitch-switch.postinst | 3 +++ >> debian/openvswitch-testcontroller.init | 3 ++- >> debian/openvswitch-testcontroller.postinst | 2 ++ >> debian/openvswitch-vtep.init | 8 +++++- >> 10 files changed, 63 insertions(+), 4 deletions(-) >> create mode 100755 debian/openvswitch-common.postinst >> >> 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. > s/Debain/Debian >> >> >> 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/openvswitch-common.postinst b/debian/openvswitch-common.postinst >> new file mode 100755 >> index 0000000..c90ab5a >> --- /dev/null >> +++ b/debian/openvswitch-common.postinst >> @@ -0,0 +1,42 @@ >> +#!/bin/sh >> +# postinst script for openvswitch-switch > Copy-paste error: This is openvswitch-common and not > openvswitch-switch postinst script > >> +# >> +# see: dh_installdeb(1) >> + >> +set -e >> + >> +# 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 || true > There are useradd and adduser utilities. I tried *bare minimum* Debian > 8 installation and adduser was not installed by default. Should you > add adduser to dependencies in debian/control file? > Good catch. I will add adduser as dependency. > >> + >> + # Fix ownership and permissions. >> + chown -R ovs:ovs $LOGDIR >> + chmod -R 0770 $LOGDIR > You have probably thought more about this, but now "adm" group is > dropped for OVS logs. Do you see any issue with this? > This is an area I'd like to get some input. Should we add ovs to the adm group by default and set the ownership of those log files to ovs:adm? > >> + ;; >> + >> + 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..030180d 100755 >> --- a/debian/openvswitch-pki.postinst >> +++ b/debian/openvswitch-pki.postinst >> @@ -31,6 +31,8 @@ case "$1" in >> if test ! -e /var/lib/openvswitch/pki; then >> ovs-pki init >> fi >> + >> + chown ovs:ovs -R /var/lib/openvswitch/pki > Shouldn't changing user recursively for /var/lib/openvswitch be a > better approach? Probably. I see that this is only package that creates /var/lib/openvswitch, but I don't see any other directory being created in addition to pki. I could be wrong since I don't install this package often. >> ;; >> >> 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..be929b6 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 ovs >> delaycompress >> missingok >> rotate 30 >> diff --git a/debian/openvswitch-switch.postinst b/debian/openvswitch-switch.postinst >> index 2464572..9183bdc 100755 >> --- a/debian/openvswitch-switch.postinst >> +++ b/debian/openvswitch-switch.postinst >> @@ -33,6 +33,9 @@ case "$1" in >> fi >> done >> fi >> + >> + # fix owner and permissions for /etc/openvswitch. >> + chown ovs:ovs -R /etc/openvswitch > > can you assume that this directory unconditionally exists (I believe > all debian scripts are run with set -e and you don't want them to > terminate prematurely)? It is listed in openvswitch-switch.dirs, not enough? > >> ;; >> >> abort-upgrade|abort-remove|abort-deconfigure) >> diff --git a/debian/openvswitch-testcontroller.init b/debian/openvswitch-testcontroller.init >> index 67b7a99..352c95d 100755 >> --- a/debian/openvswitch-testcontroller.init >> +++ b/debian/openvswitch-testcontroller.init >> @@ -109,7 +109,7 @@ start_server() { >> fi >> >> if [ ! -d /var/run/openvswitch ]; then >> - install -d -m 755 -o root -g root /var/run/openvswitch >> + install -d -m 755 -o ovs -g ovs /var/run/openvswitch > if directory exists this will not change ownership, right? Yes, This is a bug. Thanks. >> fi >> >> SSL_OPTS= >> @@ -139,6 +139,7 @@ start_server() { >> if [ -z "$DAEMONUSER" ] ; then >> start-stop-daemon --start --pidfile $PIDFILE \ >> --exec $DAEMON -- --detach --pidfile=$PIDFILE \ >> + --user ovs:ovs \ > it seems inconsistent that in some places you use --user ovs:ovs but > in other --user "$OVS_USER":"$OVS_GROUP" I used it in openvswitch-vtep.init since there are multiple references. ovs:ovs is used when it is a single change. What's your preference? > > >> $LISTEN $DAEMON_OPTS $SSL_OPTS >> errcode=$? >> else >> diff --git a/debian/openvswitch-testcontroller.postinst b/debian/openvswitch-testcontroller.postinst >> index 7242b4a..e8584e2 100755 >> --- a/debian/openvswitch-testcontroller.postinst >> +++ b/debian/openvswitch-testcontroller.postinst >> @@ -42,6 +42,8 @@ case "$1" in >> chmod go+r cert.pem req.pem >> umask $oldumask >> fi >> + >> + chown ovs:ovs -R /etc/openvswitch-testcontroller >> ;; >> >> abort-upgrade|abort-remove|abort-deconfigure) >> diff --git a/debian/openvswitch-vtep.init b/debian/openvswitch-vtep.init >> index ebf4e26..6fe02a1 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 >> >> # Include defaults if available >> default=/etc/default/openvswitch-vtep >> @@ -40,17 +42,21 @@ 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 >> + >> 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 >> } >> >> -- >> 1.9.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev
On Wed, Oct 7, 2015 at 8:20 PM, Andy Zhou <azhou@nicira.com> wrote: > On Wed, Oct 7, 2015 at 6:49 PM, Ansis Atteka <aatteka@nicira.com> wrote: >> On Mon, Oct 5, 2015 at 6:38 PM, Andy Zhou <azhou@nicira.com> wrote: >> >> Thanks Andy for doing this! I will have another more careful look at >> this patch tomorrow, because I think I somehow managed to get into a >> state where after installing debian packages /etc/openvswitch still >> belonged to root. >> > Is possible you have your selinux changes already applied? It worked > in my set up. This is Debian so SElinux (and my RHEL patch) does not come into picture here. Will spend a little more time to understand what exactly happened, but to give a hint I tried to install packages *one by one* (opposed to a single dpkg -i command) so perhaps this might have to do something with the order if it wasn't my setup itself. >> >>> 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. >> >> s/users belong/users that belong > Thanks, will fix. >> >>> Start daemons as the ovs user. >>> >>> Signed-off-by: Andy Zhou <azhou@nicira.com> >> >> This patch, I believe, breaks upgrades: >> >> Wed Oct 7 23:35:44 UTC 2015:stop >> * ovs-vswitchd is not running >> * ovsdb-server is not running >> Wed Oct 7 23:35:44 UTC 2015:load-kmod >> Wed Oct 7 23:35:44 UTC 2015:start --system-id=random --no-run-as-root >> ovsdb-server: /var/run/openvswitch/ovsdb-server.pid.tmp: create failed >> (Permission denied) >> >> I guess this was happening because that directory still belonged to >> the root user after the upgrade. >> > The error mentioned above will cause this. The error I mentioned above was for /etc/openvswitch. This is for /var/run/openvswitch/. And they happened independently. >> >>> >>> ---- >>> This patch does not include changes to the ipsec package. Ansis has >>> other plans for updating it. >> >> Yeah, I will have to figure out how to do this from Python daemons. I >> guess we have to synchronize our changes so that we don't break IPsec. >> >>> --- >>> NEWS | 3 ++- >>> debian/automake.mk | 1 + >>> debian/openvswitch-common.postinst | 42 ++++++++++++++++++++++++++++++ >>> debian/openvswitch-pki.postinst | 2 ++ >>> debian/openvswitch-switch.init | 1 + >>> debian/openvswitch-switch.logrotate | 2 +- >>> debian/openvswitch-switch.postinst | 3 +++ >>> debian/openvswitch-testcontroller.init | 3 ++- >>> debian/openvswitch-testcontroller.postinst | 2 ++ >>> debian/openvswitch-vtep.init | 8 +++++- >>> 10 files changed, 63 insertions(+), 4 deletions(-) >>> create mode 100755 debian/openvswitch-common.postinst >>> >>> 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. >> s/Debain/Debian >>> >>> >>> 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/openvswitch-common.postinst b/debian/openvswitch-common.postinst >>> new file mode 100755 >>> index 0000000..c90ab5a >>> --- /dev/null >>> +++ b/debian/openvswitch-common.postinst >>> @@ -0,0 +1,42 @@ >>> +#!/bin/sh >>> +# postinst script for openvswitch-switch >> Copy-paste error: This is openvswitch-common and not >> openvswitch-switch postinst script >> >>> +# >>> +# see: dh_installdeb(1) >>> + >>> +set -e >>> + >>> +# 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 || true >> There are useradd and adduser utilities. I tried *bare minimum* Debian >> 8 installation and adduser was not installed by default. Should you >> add adduser to dependencies in debian/control file? >> > Good catch. I will add adduser as dependency. >> >>> + >>> + # Fix ownership and permissions. >>> + chown -R ovs:ovs $LOGDIR >>> + chmod -R 0770 $LOGDIR >> You have probably thought more about this, but now "adm" group is >> dropped for OVS logs. Do you see any issue with this? >> > This is an area I'd like to get some input. Should we add ovs to the > adm group by default and > set the ownership of those log files to ovs:adm? This is the best I could find: http://serverfault.com/questions/485473/what-is-the-canonical-use-for-the-sys-and-adm-groups Basically after your patch I can't see /var/log/openvswitch/ovs-vswitchd.log anymore with my regular Ubuntu user. However, on Ubuntu there is already precedent with speech-dispatcher that has the same behavior: aatteka@aatteka-MacBookPro:/var/log$ cat speech-dispatcher/ cat: speech-dispatcher/: Permission denied >> >>> + ;; >>> + >>> + 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..030180d 100755 >>> --- a/debian/openvswitch-pki.postinst >>> +++ b/debian/openvswitch-pki.postinst >>> @@ -31,6 +31,8 @@ case "$1" in >>> if test ! -e /var/lib/openvswitch/pki; then >>> ovs-pki init >>> fi >>> + >>> + chown ovs:ovs -R /var/lib/openvswitch/pki >> Shouldn't changing user recursively for /var/lib/openvswitch be a >> better approach? > Probably. I see that this is only package that creates > /var/lib/openvswitch, but I don't see any other directory > being created in addition to pki. I could be wrong since I don't > install this package often. I am a bit afraid that we have plenty of OVS packages creating sub-directories in other package directories (either from debian.dirs file, postinst script, init.d script or the daemon itself). I really don't have a good alternative, but this is something, I am afraid, could get easily out of hand and become hard to maintain, if we will be calling mkdir and chown from various places. >>> ;; >>> >>> 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..be929b6 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 ovs >>> delaycompress >>> missingok >>> rotate 30 >>> diff --git a/debian/openvswitch-switch.postinst b/debian/openvswitch-switch.postinst >>> index 2464572..9183bdc 100755 >>> --- a/debian/openvswitch-switch.postinst >>> +++ b/debian/openvswitch-switch.postinst >>> @@ -33,6 +33,9 @@ case "$1" in >>> fi >>> done >>> fi >>> + >>> + # fix owner and permissions for /etc/openvswitch. >>> + chown ovs:ovs -R /etc/openvswitch >> >> can you assume that this directory unconditionally exists (I believe >> all debian scripts are run with set -e and you don't want them to >> terminate prematurely)? > It is listed in openvswitch-switch.dirs, not enough? I think yesterday offline we discussed something among the lines where OVS should be able to handle gracefully corner case scenario where unknowing user deletes /etc/openvswitch directory. I think we got into this discussion because we tried to understand rationale on why OVS init.d script creates /etc/openvswitch directory if it is not present already. aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ sudo rm -rf /etc/openvswitch/ aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ sudo dpkg-reconfigure openvswitch-switch chown: cannot access ‘/etc/openvswitch’: No such file or directory Overall, I think we should have a clear stance on what OVS should do in such corner cases - either attempt to recover system or tell user that if he deleted /etc/openvswitch directory then he should figure out on his own how to recover setup. I haven't looked in Debian Maintainerš book if it has any recommendations about this subject. >> >>> ;; >>> >>> abort-upgrade|abort-remove|abort-deconfigure) >>> diff --git a/debian/openvswitch-testcontroller.init b/debian/openvswitch-testcontroller.init >>> index 67b7a99..352c95d 100755 >>> --- a/debian/openvswitch-testcontroller.init >>> +++ b/debian/openvswitch-testcontroller.init >>> @@ -109,7 +109,7 @@ start_server() { >>> fi >>> >>> if [ ! -d /var/run/openvswitch ]; then >>> - install -d -m 755 -o root -g root /var/run/openvswitch >>> + install -d -m 755 -o ovs -g ovs /var/run/openvswitch >> if directory exists this will not change ownership, right? > Yes, This is a bug. Thanks. >>> fi >>> >>> SSL_OPTS= >>> @@ -139,6 +139,7 @@ start_server() { >>> if [ -z "$DAEMONUSER" ] ; then By they way DAEMONUSER and OVS_USER seem to replicate the same use-cases. Do we really need both? >>> start-stop-daemon --start --pidfile $PIDFILE \ >>> --exec $DAEMON -- --detach --pidfile=$PIDFILE \ >>> + --user ovs:ovs \ >> it seems inconsistent that in some places you use --user ovs:ovs but >> in other --user "$OVS_USER":"$OVS_GROUP" > I used it in openvswitch-vtep.init since there are multiple > references. ovs:ovs is used when > it is a single change. What's your preference? each time you hard code "ovs" (or "ovs:ovs") user you are repeating yourself. The other place in this postinst script, I believe, is "install" command: aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ cat debian/openvswitch-testcontroller.init | grep ovs 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 install -d -m 755 -o ovs -g ovs /var/run/openvswitch --user ovs:ovs \ Here are my preferences: 1. define "ovs" user and group in one place and somehow pass it to all postinst scripts. So that if one day you would need to change user from "ovs" to something else a single line change would be sufficient. 2. define "ovs" user and group at most once in each postinst script. So that if one day you would need to change user from "ovs" to something else a single line change in each postinst script would be sufficient. 3. define "ovs" user and group inline or sometimes in postinst script. > >> >> >>> $LISTEN $DAEMON_OPTS $SSL_OPTS >>> errcode=$? >>> else >>> diff --git a/debian/openvswitch-testcontroller.postinst b/debian/openvswitch-testcontroller.postinst >>> index 7242b4a..e8584e2 100755 >>> --- a/debian/openvswitch-testcontroller.postinst >>> +++ b/debian/openvswitch-testcontroller.postinst >>> @@ -42,6 +42,8 @@ case "$1" in >>> chmod go+r cert.pem req.pem >>> umask $oldumask >>> fi >>> + >>> + chown ovs:ovs -R /etc/openvswitch-testcontroller >>> ;; >>> >>> abort-upgrade|abort-remove|abort-deconfigure) >>> diff --git a/debian/openvswitch-vtep.init b/debian/openvswitch-vtep.init >>> index ebf4e26..6fe02a1 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 >>> >>> # Include defaults if available >>> default=/etc/default/openvswitch-vtep >>> @@ -40,17 +42,21 @@ 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 >>> + >>> 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 >>> } >>> >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> http://openvswitch.org/mailman/listinfo/dev
On Thu, Oct 8, 2015 at 4:01 PM, Ansis Atteka <aatteka@nicira.com> wrote: > On Wed, Oct 7, 2015 at 8:20 PM, Andy Zhou <azhou@nicira.com> wrote: >> On Wed, Oct 7, 2015 at 6:49 PM, Ansis Atteka <aatteka@nicira.com> wrote: >>> On Mon, Oct 5, 2015 at 6:38 PM, Andy Zhou <azhou@nicira.com> wrote: >>> >>> Thanks Andy for doing this! I will have another more careful look at >>> this patch tomorrow, because I think I somehow managed to get into a >>> state where after installing debian packages /etc/openvswitch still >>> belonged to root. >>> >> Is possible you have your selinux changes already applied? It worked >> in my set up. > > This is Debian so SElinux (and my RHEL patch) does not come into > picture here. Will spend a little more time to understand what exactly > happened, but to give a hint I tried to install packages *one by one* > (opposed to a single dpkg -i command) so perhaps this might have to do > something with the order if it wasn't my setup itself. > >>> >>>> 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. >>> >>> s/users belong/users that belong >> Thanks, will fix. >>> >>>> Start daemons as the ovs user. >>>> >>>> Signed-off-by: Andy Zhou <azhou@nicira.com> >>> >>> This patch, I believe, breaks upgrades: >>> >>> Wed Oct 7 23:35:44 UTC 2015:stop >>> * ovs-vswitchd is not running >>> * ovsdb-server is not running >>> Wed Oct 7 23:35:44 UTC 2015:load-kmod >>> Wed Oct 7 23:35:44 UTC 2015:start --system-id=random --no-run-as-root >>> ovsdb-server: /var/run/openvswitch/ovsdb-server.pid.tmp: create failed >>> (Permission denied) >>> >>> I guess this was happening because that directory still belonged to >>> the root user after the upgrade. >>> >> The error mentioned above will cause this. > > The error I mentioned above was for /etc/openvswitch. This is for > /var/run/openvswitch/. And they happened independently. I think I found the bug; it should be fixed in v2. > >>> >>>> >>>> ---- >>>> This patch does not include changes to the ipsec package. Ansis has >>>> other plans for updating it. >>> >>> Yeah, I will have to figure out how to do this from Python daemons. I >>> guess we have to synchronize our changes so that we don't break IPsec. >>> >>>> --- >>>> NEWS | 3 ++- >>>> debian/automake.mk | 1 + >>>> debian/openvswitch-common.postinst | 42 ++++++++++++++++++++++++++++++ >>>> debian/openvswitch-pki.postinst | 2 ++ >>>> debian/openvswitch-switch.init | 1 + >>>> debian/openvswitch-switch.logrotate | 2 +- >>>> debian/openvswitch-switch.postinst | 3 +++ >>>> debian/openvswitch-testcontroller.init | 3 ++- >>>> debian/openvswitch-testcontroller.postinst | 2 ++ >>>> debian/openvswitch-vtep.init | 8 +++++- >>>> 10 files changed, 63 insertions(+), 4 deletions(-) >>>> create mode 100755 debian/openvswitch-common.postinst >>>> >>>> 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. >>> s/Debain/Debian >>>> >>>> >>>> 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/openvswitch-common.postinst b/debian/openvswitch-common.postinst >>>> new file mode 100755 >>>> index 0000000..c90ab5a >>>> --- /dev/null >>>> +++ b/debian/openvswitch-common.postinst >>>> @@ -0,0 +1,42 @@ >>>> +#!/bin/sh >>>> +# postinst script for openvswitch-switch >>> Copy-paste error: This is openvswitch-common and not >>> openvswitch-switch postinst script >>> >>>> +# >>>> +# see: dh_installdeb(1) >>>> + >>>> +set -e >>>> + >>>> +# 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 || true >>> There are useradd and adduser utilities. I tried *bare minimum* Debian >>> 8 installation and adduser was not installed by default. Should you >>> add adduser to dependencies in debian/control file? >>> >> Good catch. I will add adduser as dependency. >>> >>>> + >>>> + # Fix ownership and permissions. >>>> + chown -R ovs:ovs $LOGDIR >>>> + chmod -R 0770 $LOGDIR >>> You have probably thought more about this, but now "adm" group is >>> dropped for OVS logs. Do you see any issue with this? >>> >> This is an area I'd like to get some input. Should we add ovs to the >> adm group by default and >> set the ownership of those log files to ovs:adm? > > This is the best I could find: > http://serverfault.com/questions/485473/what-is-the-canonical-use-for-the-sys-and-adm-groups > > Basically after your patch I can't see > /var/log/openvswitch/ovs-vswitchd.log anymore with my regular Ubuntu > user. However, on Ubuntu there is already precedent with > speech-dispatcher that has the same behavior: Before my changes /var/log/openvswitch was set to allow read access by "others'. The v2 just posted keeps this setting. > > aatteka@aatteka-MacBookPro:/var/log$ cat speech-dispatcher/ > cat: speech-dispatcher/: Permission denied > > >>> >>>> + ;; >>>> + >>>> + 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..030180d 100755 >>>> --- a/debian/openvswitch-pki.postinst >>>> +++ b/debian/openvswitch-pki.postinst >>>> @@ -31,6 +31,8 @@ case "$1" in >>>> if test ! -e /var/lib/openvswitch/pki; then >>>> ovs-pki init >>>> fi >>>> + >>>> + chown ovs:ovs -R /var/lib/openvswitch/pki >>> Shouldn't changing user recursively for /var/lib/openvswitch be a >>> better approach? >> Probably. I see that this is only package that creates >> /var/lib/openvswitch, but I don't see any other directory >> being created in addition to pki. I could be wrong since I don't >> install this package often. > > I am a bit afraid that we have plenty of OVS packages creating > sub-directories in other package directories (either from debian.dirs > file, postinst script, init.d script or the daemon itself). I really > don't have a good alternative, but this is something, I am afraid, > could get easily out of hand and become hard to maintain, if we will > be calling mkdir and chown from various places. > >>>> ;; >>>> >>>> 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..be929b6 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 ovs >>>> delaycompress >>>> missingok >>>> rotate 30 >>>> diff --git a/debian/openvswitch-switch.postinst b/debian/openvswitch-switch.postinst >>>> index 2464572..9183bdc 100755 >>>> --- a/debian/openvswitch-switch.postinst >>>> +++ b/debian/openvswitch-switch.postinst >>>> @@ -33,6 +33,9 @@ case "$1" in >>>> fi >>>> done >>>> fi >>>> + >>>> + # fix owner and permissions for /etc/openvswitch. >>>> + chown ovs:ovs -R /etc/openvswitch >>> >>> can you assume that this directory unconditionally exists (I believe >>> all debian scripts are run with set -e and you don't want them to >>> terminate prematurely)? >> It is listed in openvswitch-switch.dirs, not enough? > > I think yesterday offline we discussed something among the lines where > OVS should be able to handle gracefully corner case scenario where > unknowing user deletes /etc/openvswitch directory. I think we got into > this discussion because we tried to understand rationale on why OVS > init.d script creates /etc/openvswitch directory if it is not present > already. > > aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ sudo rm -rf /etc/openvswitch/ > aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ sudo dpkg-reconfigure > openvswitch-switch > chown: cannot access ‘/etc/openvswitch’: No such file or directory > > Overall, I think we should have a clear stance on what OVS should do > in such corner cases - either attempt to recover system or tell user > that if he deleted /etc/openvswitch directory then he should figure > out on his own how to recover setup. I haven't looked in Debian > Maintainerš book if it has any recommendations about this subject. I think SElinux may post a stricter restriction on what script can do. I think we should revisit this taking SElinux into consideration. v2 changes does not fully address those issues yet. > >>> >>>> ;; >>>> >>>> abort-upgrade|abort-remove|abort-deconfigure) >>>> diff --git a/debian/openvswitch-testcontroller.init b/debian/openvswitch-testcontroller.init >>>> index 67b7a99..352c95d 100755 >>>> --- a/debian/openvswitch-testcontroller.init >>>> +++ b/debian/openvswitch-testcontroller.init >>>> @@ -109,7 +109,7 @@ start_server() { >>>> fi >>>> >>>> if [ ! -d /var/run/openvswitch ]; then >>>> - install -d -m 755 -o root -g root /var/run/openvswitch >>>> + install -d -m 755 -o ovs -g ovs /var/run/openvswitch >>> if directory exists this will not change ownership, right? >> Yes, This is a bug. Thanks. >>>> fi >>>> >>>> SSL_OPTS= >>>> @@ -139,6 +139,7 @@ start_server() { >>>> if [ -z "$DAEMONUSER" ] ; then > > By they way DAEMONUSER and OVS_USER seem to replicate the same > use-cases. Do we really need both? > >>>> start-stop-daemon --start --pidfile $PIDFILE \ >>>> --exec $DAEMON -- --detach --pidfile=$PIDFILE \ >>>> + --user ovs:ovs \ >>> it seems inconsistent that in some places you use --user ovs:ovs but >>> in other --user "$OVS_USER":"$OVS_GROUP" >> I used it in openvswitch-vtep.init since there are multiple >> references. ovs:ovs is used when >> it is a single change. What's your preference? > > each time you hard code "ovs" (or "ovs:ovs") user you are repeating > yourself. The other place in this postinst script, I believe, is > "install" command: > > aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ cat > debian/openvswitch-testcontroller.init | grep ovs > 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 > install -d -m 755 -o ovs -g ovs /var/run/openvswitch > --user ovs:ovs \ > > Here are my preferences: > 1. define "ovs" user and group in one place and somehow pass it to all > postinst scripts. So that if one day you would need to change user > from "ovs" to something else a single line change would be sufficient. > 2. define "ovs" user and group at most once in each postinst script. > So that if one day you would need to change user from "ovs" to > something else a single line change in each postinst script would be > sufficient. > 3. define "ovs" user and group inline or sometimes in postinst script. I have adopted #2 for V2. I can't figure out a clean way to implement #1. Overall, thanks for testing out the changes and feedbacks. They are very helpful.
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/openvswitch-common.postinst b/debian/openvswitch-common.postinst new file mode 100755 index 0000000..c90ab5a --- /dev/null +++ b/debian/openvswitch-common.postinst @@ -0,0 +1,42 @@ +#!/bin/sh +# postinst script for openvswitch-switch +# +# see: dh_installdeb(1) + +set -e + +# 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 || true + + # Fix ownership and permissions. + chown -R ovs:ovs $LOGDIR + chmod -R 0770 $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..030180d 100755 --- a/debian/openvswitch-pki.postinst +++ b/debian/openvswitch-pki.postinst @@ -31,6 +31,8 @@ case "$1" in if test ! -e /var/lib/openvswitch/pki; then ovs-pki init fi + + chown ovs:ovs -R /var/lib/openvswitch/pki ;; 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..be929b6 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 ovs delaycompress missingok rotate 30 diff --git a/debian/openvswitch-switch.postinst b/debian/openvswitch-switch.postinst index 2464572..9183bdc 100755 --- a/debian/openvswitch-switch.postinst +++ b/debian/openvswitch-switch.postinst @@ -33,6 +33,9 @@ case "$1" in fi done fi + + # fix owner and permissions for /etc/openvswitch. + chown ovs:ovs -R /etc/openvswitch ;; abort-upgrade|abort-remove|abort-deconfigure) diff --git a/debian/openvswitch-testcontroller.init b/debian/openvswitch-testcontroller.init index 67b7a99..352c95d 100755 --- a/debian/openvswitch-testcontroller.init +++ b/debian/openvswitch-testcontroller.init @@ -109,7 +109,7 @@ start_server() { fi if [ ! -d /var/run/openvswitch ]; then - install -d -m 755 -o root -g root /var/run/openvswitch + install -d -m 755 -o ovs -g ovs /var/run/openvswitch fi SSL_OPTS= @@ -139,6 +139,7 @@ start_server() { if [ -z "$DAEMONUSER" ] ; then start-stop-daemon --start --pidfile $PIDFILE \ --exec $DAEMON -- --detach --pidfile=$PIDFILE \ + --user ovs:ovs \ $LISTEN $DAEMON_OPTS $SSL_OPTS errcode=$? else diff --git a/debian/openvswitch-testcontroller.postinst b/debian/openvswitch-testcontroller.postinst index 7242b4a..e8584e2 100755 --- a/debian/openvswitch-testcontroller.postinst +++ b/debian/openvswitch-testcontroller.postinst @@ -42,6 +42,8 @@ case "$1" in chmod go+r cert.pem req.pem umask $oldumask fi + + chown ovs:ovs -R /etc/openvswitch-testcontroller ;; abort-upgrade|abort-remove|abort-deconfigure) diff --git a/debian/openvswitch-vtep.init b/debian/openvswitch-vtep.init index ebf4e26..6fe02a1 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 # Include defaults if available default=/etc/default/openvswitch-vtep @@ -40,17 +42,21 @@ 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 + 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/openvswitch-common.postinst | 42 ++++++++++++++++++++++++++++++ debian/openvswitch-pki.postinst | 2 ++ debian/openvswitch-switch.init | 1 + debian/openvswitch-switch.logrotate | 2 +- debian/openvswitch-switch.postinst | 3 +++ debian/openvswitch-testcontroller.init | 3 ++- debian/openvswitch-testcontroller.postinst | 2 ++ debian/openvswitch-vtep.init | 8 +++++- 10 files changed, 63 insertions(+), 4 deletions(-) create mode 100755 debian/openvswitch-common.postinst