On 10 Feb 2023, at 17:06, Vladislav Odintsov wrote:
> Thanks Eelco,
>
> requested changes addressed here:
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/20230210160229.3689298-1-odiv...@gmail.com/
>
Thanks for the heads up. I will try to take a loot at it next week.
//Eelco
>> On 10 Feb 2023, at 11:21, Eelco Chaudron <echau...@redhat.com> wrote:
>>
>>
>>
>> On 8 Feb 2023, at 16:22, Vladislav Odintsov wrote:
>>
>>> This patch adds new ovs-ctl options to pass umask configuration to allow
>>> OVS daemons set requested socket permissions on group. Previous
>>> behaviour (if using with systemd service unit) created sockets with 0750
>>> permissions mask (group has no write permission).
>>>
>>> Write permission for group is reasonable in usecase, where ovs-vswitchd
>>> or ovsdb-server runs as a non-privileged user:group (say,
>>> openvswitch:openvswitch) and it is needed to access unix socket from
>>> process running as another non-privileged user. In this case
>>> administrator has to add that user to openvswitch group and can connect
>>> to OVS sockets from a process running under that user.
>>>
>>> Two new ovs-ctl options --ovsdb-server-umask and --ovs-vswitchd-umask
>>> were added to manage umask values for appropriate daemons. This is
>>> useful for systemd users: both ovs-vswitchd and ovsdb-server systemd
>>> units read options from single /etc/sysconfig/openvswitch configuration
>>> file. So, with separate options it is possible to set umask only for
>>> specific daemon.
>>>
>>> OPTIONS="--ovsdb-server-umask=0002"
>>>
>>> in /etc/openvswitch/sysconfig file will set umask to 0002 value before
>>> starting only ovsdb-server, while
>>>
>>> OPTIONS="--ovs-vswitchd-umask=0002"
>>>
>>> will set umask to ovs-vswitchd daemon.
>>>
>>> Previous behaviour (not setting umask) is left as default.
>>
>> Thanks for reworking the patch, small none technical nit below, the rest
>> looks good.
>>
>> //Eelco
>>
>>
>>> Reported-at:
>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401501.html
>>> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com>
>>>
>>> ---
>>> v1 -> v2:
>>> - added item in NEWS file as Ilya's suggestion;
>>> - addressed Eelco's review comments;
>>> - moved umask call from ovs-ctl to ovs-lib;
>>> - added restoration of umask to effective value before the umask change;
>>> - previous version --ovs-umask option was split into two:
>>> --ovs-vswitchd-umask and --ovsdb-server-umask in order to make
>>> possible umask configuration for specific daemon when running with
>>> systemd.
>>>
>>> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com>
>>> ---
>>> NEWS | 7 +++++++
>>> utilities/ovs-ctl.in | 16 ++++++++++++----
>>> utilities/ovs-lib.in | 17 ++++++++++++++---
>>> 3 files changed, 33 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index fe6055a27..f7df598bd 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -4,6 +4,13 @@ Post-v3.1.0
>>> * OVS now collects per-interface upcall statistics that can be obtained
>>> via 'ovs-appctl dpctl/show -s' or the interface's statistics column
>>> in OVSDB. Available with upstream kernel 6.2+.
>>> + - ovs-ctl:
>>> + * Added support to set umask value when starting OVS daemons. New
>>> options
>>> + --ovsdb-server-umask=MODE and --ovs-vswitchd-umask=MODE were added
>>> for
>>> + that. For instance, when write access on befalf of OVS group is
>>> needed
>>> + for ovsdb-server, pass --ovsdb-umask=0002. Use --vswitchd-umask to
>>> set
>>> + umask ovs-vswitchd daemon umask. This will allow ovsdb-server or
>>> + ovs-vswitchd to create sockets with access mode of 0770.
>>>
>>>
>>> v3.1.0 - xx xxx xxxx
>>> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
>>> index d91552588..0b2820c36 100644
>>> --- a/utilities/ovs-ctl.in
>>> +++ b/utilities/ovs-ctl.in
>>> @@ -156,8 +156,8 @@ do_start_ovsdb () {
>>> [ "$OVS_USER" != "" ] && set "$@" --user "$OVS_USER"
>>> [ "$OVSDB_SERVER_OPTIONS" != "" ] && set "$@" $OVSDB_SERVER_OPTIONS
>>>
>>> - start_daemon "$OVSDB_SERVER_PRIORITY" "$OVSDB_SERVER_WRAPPER" "$@"
>>> \
>>> - || return 1
>>> + start_daemon "$OVSDB_SERVER_PRIORITY" "$OVSDB_SERVER_WRAPPER" \
>>> + "$OVSDB_SERVER_UMASK" "$@" || return 1
>>>
>>> # Initialize database settings.
>>> ovs_vsctl -- init -- set Open_vSwitch . db-version="$schemaver" \
>>> @@ -226,8 +226,8 @@ do_start_forwarding () {
>>> [ "$OVS_USER" != "" ] && set "$@" --user "$OVS_USER"
>>> [ "$OVS_VSWITCHD_OPTIONS" != "" ] &&set "$@" $OVS_VSWITCHD_OPTIONS
>>>
>>> - start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" "$@"
>>> ||
>>> - return 1
>>> + start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" \
>>> + "$OVS_VSWITCHD_UMASK" "$@" || return 1
>>> fi
>>> }
>>>
>>> @@ -348,6 +348,8 @@ set_defaults () {
>>> OVS_VSWITCHD_WRAPPER=
>>> OVSDB_SERVER_OPTIONS=
>>> OVS_VSWITCHD_OPTIONS=
>>> + OVSDB_SERVER_UMASK=
>>> + OVS_VSWITCHD_UMASK=
>>>
>>> DB_FILE=$dbdir/conf.db
>>> DB_SOCK=$rundir/db.sock
>>> @@ -421,6 +423,12 @@ Other important options for "start", "restart" and
>>> "force-reload-kmod":
>>> add given key-value pair to Open_vSwitch external-ids
>>> --delete-bridges delete all bridges just before starting ovs-vswitchd
>>> --ovs-user="user[:group]" pass the --user flag to ovs daemons
>>> + --ovsdb-server-umask=MODE Set umask prior to run ovsdb-server daemon.
>>> + This is useful to manage daemon's sockets
>>> permissions.
>>> + Default is not to change umask (inherited
>>> from shell).
>>> + --ovs-vswitchd-umask=MODE Set umask prior to run ovs-vswitchd daemon.
>>> + This is useful to manage daemon's sockets
>>> permissions.
>>> + Default is not to change umask (inherited
>>> from shell).
>>>
>>> Less important options for "start", "restart" and "force-reload-kmod":
>>> --daemon-cwd=DIR set working dir for OVS daemons (default:
>>> $DAEMON_CWD)
>>> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
>>> index 13477a6a9..004de631c 100644
>>> --- a/utilities/ovs-lib.in
>>> +++ b/utilities/ovs-lib.in
>>> @@ -165,9 +165,9 @@ install_dir () {
>>> }
>>>
>>> start_daemon () {
>>> - priority=$1
>>> - wrapper=$2
>>> - shift; shift
>>> + priority=$1 && shift
>>> + wrapper=$1 && shift
>>> + umask=$1 && shift
>>> daemon=$1
>>> strace=""
>>>
>>> @@ -223,8 +223,19 @@ start_daemon () {
>>> set nice -n "$priority" "$@"
>>> fi
>>>
>>> + # set requested umask if any and turn previous value back
>>
>> Please use a capital S for Set and end with a dot.
>>
>>> + if [ -n "$umask" ]; then
>>> + previuos_umask_value=$(umask)
>>> + umask "$umask"
>>> + fi
>>> +
>>> action "Starting $daemon" "$@" || return 1
>>>
>>> + # if umask was set, turn umask value to previous value
>>
>> Please use a capital I for If and end with a dot.
>>
>>> + if [ -n "$umask" ]; then
>>> + umask "$previuos_umask_value"
>>> + fi
>>> +
>>> if test X"$strace" != X; then
>>> # Strace doesn't have the -D option so we attach after the fact.
>>> setsid $strace -o "$logdir/$daemon.strace.log" \
>>> --
>>> 2.36.1
>>>
>>> _______________________________________________
>>> dev mailing list
>>> d...@openvswitch.org <mailto:d...@openvswitch.org>
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org <mailto:d...@openvswitch.org>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
> Regards,
> Vladislav Odintsov
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
