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). > >> 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 "$@". 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? > >> + >> 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
