On 7 Feb 2023, at 16:04, Vladislav Odintsov wrote:
> Thanks for the review, Eelco! > Answers are inline. > >> On 7 Feb 2023, at 17:02, Eelco Chaudron <[email protected]> wrote: >> >> See some comments inline below. >> >> Cheers, >> >> Eelco >> >> On 27 Jan 2023, at 14:29, Vladislav Odintsov wrote: >> >>> This patch adds a new ovs-ctl option to pass umask configuration to allow >>> OVS daemons to 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 that user. >> >> Will this affect any other files generated by vswitchd (logs?) that are now >> accessible by unprivileged users? > > I’ve checked other files: pidfile, logs, ovsdb lock files and see no > difference between default behaviour (no umask call) and when umask is set to > 0002, except for socket files: > pid files: 0644 > log files: 0640 > ovsdb lock: 0600 > socket files have 0750 as a default value and 0770 if umask 000X is used (X > could be any value in the 0-7 range, if IIC). I did not find anything quickly that could be a loose end... Maybe someone else has some ideas. >> >>> Previous behaviour (not setting umask) is left as default. >>> >>> Reported-at: >>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401501.html >>> Signed-off-by: Vladislav Odintsov <[email protected]> >>> --- >>> utilities/ovs-ctl.in | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in >>> index e6e07f476..b97d568c6 100644 >>> --- a/utilities/ovs-ctl.in >>> +++ b/utilities/ovs-ctl.in >>> @@ -334,6 +334,7 @@ set_defaults () { >>> SELF_CONFINEMENT=yes >>> MONITOR=yes >>> OVS_USER= >>> + OVS_UMASK= >>> OVSDB_SERVER=yes >>> OVS_VSWITCHD=yes >>> OVSDB_SERVER_PRIORITY=-10 >>> @@ -415,6 +416,8 @@ 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 >>> + --ovs-umask=XXXX Set umask prior to run OVS daemons. >> >> Rather than XXXX you might want to use MODE. > > Agree. Will address this in v2. > >> >>> + This is needed to manage socket group permissions. >>> >>> Less important options for "start", "restart" and "force-reload-kmod": >>> --daemon-cwd=DIR set working dir for OVS daemons (default: >>> $DAEMON_CWD) >>> @@ -542,6 +545,11 @@ do >>> ;; >>> esac >>> done >>> + >>> +if [ -n "$OVS_UMASK" ]; then >>> + umask "$OVS_UMASK" >>> +fi >> >> The help mentions the list of commands are important >> start/restart/force-reload-kmod commands, but you set this umask for any >> command. >> Actually, the entire script is now executed with this umask, not only the >> daemons, if this is intended you might want to highlight it. Did not check >> if there is any implication for doing this. > > Hmm, nice point. > Maybe it would be better to move setting umask to ovs-lib.in file and pass > umask value to start_daemon() function? > It could be placed under setting nice value., right before running process > with "$@". Sound like a nice place to add this, also you need to restore it after running the daemon. > Also, we could split --ovs-umask option into two: > --ovsdb-umask, which sets umask only before ovsdb server start and > --vswitchd-umask, which sets umask only before ovs-vswitchd daemon start. > > What do you think? I guess a global value would still work, as you can use --no-ovs-vswitchd and --no-ovsdb-server and split the startup over two commands if you really need different umasks. But I’m fine with either way, maybe Ilya has any preference. >> >>> + >>> case $command in >>> start) >>> start_ovsdb || exit 1 >>> -- >>> 2.36.1 >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] <mailto:[email protected]> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> _______________________________________________ >> dev mailing list >> [email protected] <mailto:[email protected]> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Regards, > Vladislav Odintsov _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
