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

Message ID 1444095524-11357-4-git-send-email-azhou@nicira.com
State Superseded
Headers show

Commit Message

Andy Zhou Oct. 6, 2015, 1:38 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/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

Comments

Ansis Atteka Oct. 8, 2015, 1:49 a.m. UTC | #1
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
Andy Zhou Oct. 8, 2015, 3:20 a.m. UTC | #2
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
Ansis Atteka Oct. 8, 2015, 11:01 p.m. UTC | #3
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
Andy Zhou Oct. 10, 2015, 12:24 a.m. UTC | #4
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.

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/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
 }