| Message ID | 1441323223-11889-2-git-send-email-azhou@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 3 September 2015 at 16:33, Andy Zhou <azhou@nicira.com> wrote: > Allow daemon running as root to accept --user option, that accepts > "user:group" string as input. Performs sanity check on the input, > and store the converted uid and gid. > > daemon_become_new_user() needs to be called to make the actual > switch. > Signed-off-by: Andy Zhou <azhou@nicira.com> > --- > lib/daemon-unix.c | 71 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > lib/daemon.h | 27 +++++++++++++++------ > 2 files changed, 91 insertions(+), 7 deletions(-) > > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > index d52ac2d..44eb800 100644 > --- a/lib/daemon-unix.c > +++ b/lib/daemon-unix.c > @@ -20,6 +20,7 @@ > #include <errno.h> > #include <fcntl.h> > #include <grp.h> > +#include <pwd.h> > #include <signal.h> > #include <stdlib.h> > #include <string.h> > @@ -693,6 +694,76 @@ should_service_stop(void) > return false; > } > > +void daemon_set_new_user(const char *user_spec) > +{ > + char *pos = strchr(user_spec, ':'); > + > + uid = getuid(); > + gid = getgid(); > + > + if (gid || uid) { > + VLOG_FATAL("%s: only root can use --user option", pidfile); > + } > + > + user_spec += strspn(user_spec, " \t\r\n"); > + int len = pos ? pos - user_spec : strlen(user_spec); > + struct passwd pwd, *res; > + char buf[4096]; > + > + if (len) { > + user = xzalloc(len + 1); > + strncpy(user, user_spec, len); > + > + if (getpwnam_r(user, &pwd, buf, 4096, &res)) { > instead of using 4096 I would use "sizeof buf" here and in other places. However, from where did you get this "4096" number in the first place? I see that in the GETPWNAM man page the documented way to get the expected buffer size is: sysconf(_SC_GETPW_R_SIZE_MAX); Perhaps there is a reason you are not using that approach? > + VLOG_FATAL("%s: Invalid --user option %s (no such user %s)", > + pidfile, user_spec, user); > + } > + } else { > + /* User is not specified, use current user. */ > + if (getpwuid_r(uid, &pwd, buf, 4096, &res)) { > + VLOG_FATAL("%s: Invalid --user option %s (failed to lookup " > + "current user with uid %d)", pidfile, user_spec, > uid); > + } > + user = strdup(pwd.pw_name); > + } > + > + uid = pwd.pw_uid; > + gid = pwd.pw_gid; > + > + if (pos) { > + char *grpstr = pos + 1; > + grpstr += strspn(grpstr, " \t\r\n"); > + > + if (*grpstr) { > + struct group grp, *res; > + > + if(getgrnam_r(grpstr, &grp, buf, 4096, &res)) { > + VLOG_FATAL("%s: Invalid --user option %s (unknown group > %s)", > + pidfile, user_spec, grpstr); > + } > + > + if(gid != grp.gr_gid) { > + char **mem; > + > + for(mem = grp.gr_mem; *mem; ++mem) { > + if (!strcmp(*mem, user)) { > + break; > + } > + } > + > + if (!*mem) { > + VLOG_FATAL("%s: Invalid --user option %s (user %s is " > + "not in group %s)", pidfile, user_spec, > + user, grpstr); > + } > + gid = grp.gr_gid; > + } > + } > + } > + > + switch_to_new_user = true; > +} > + > void > daemon_become_new_user(void) > { > diff --git a/lib/daemon.h b/lib/daemon.h > index fb97cde..4b25f46 100644 > --- a/lib/daemon.h > +++ b/lib/daemon.h > @@ -42,14 +42,16 @@ > OPT_NO_CHDIR, \ > OPT_OVERWRITE_PIDFILE, \ > OPT_PIDFILE, \ > - OPT_MONITOR > + OPT_MONITOR, \ > + OPT_USER_GROUP > > -#define DAEMON_LONG_OPTIONS \ > - {"detach", no_argument, NULL, OPT_DETACH}, \ > - {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, \ > - {"pidfile", optional_argument, NULL, OPT_PIDFILE}, \ > +#define DAEMON_LONG_OPTIONS \ > + {"detach", no_argument, NULL, OPT_DETACH}, \ > + {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, \ > + {"pidfile", optional_argument, NULL, OPT_PIDFILE}, \ > {"overwrite-pidfile", no_argument, NULL, OPT_OVERWRITE_PIDFILE}, \ > - {"monitor", no_argument, NULL, OPT_MONITOR} > + {"monitor", no_argument, NULL, OPT_MONITOR}, \ > + {"user", required_argument, NULL, OPT_USER_GROUP} > > #define DAEMON_OPTION_HANDLERS \ > case OPT_DETACH: \ > @@ -70,6 +72,10 @@ > \ > case OPT_MONITOR: \ > daemon_set_monitor(); \ > + break; \ > + \ > + case OPT_USER_GROUP: \ > + daemon_set_new_user(optarg); \ > break; > > void set_detach(void); > @@ -77,6 +83,7 @@ void daemon_set_monitor(void); > void set_no_chdir(void); > void ignore_existing_pidfile(void); > void daemon_become_new_user(void); > +void daemon_set_new_user(const char *); > pid_t read_pidfile(const char *name); > #else > #define DAEMON_OPTION_ENUMS \ > @@ -85,7 +92,7 @@ pid_t read_pidfile(const char *name); > OPT_PIDFILE, \ > OPT_PIPE_HANDLE, \ > OPT_SERVICE, \ > - OPT_SERVICE_MONITOR > + OPT_SERVICE_MONITOR \ > > #define DAEMON_LONG_OPTIONS > \ > {"detach", no_argument, NULL, OPT_DETACH}, > \ > @@ -120,6 +127,12 @@ void control_handler(DWORD request); > void set_pipe_handle(const char *pipe_handle); > > static inline void > +daemon_set_new_user(const char *) > +{ > + /* Not implemented. */ > +} > + > +static inline void > daemon_become_new_user(void) > { > /* Not implemented. */ > -- > > I was able to downgrade ovsdb-server to a non-root user (aatteka in this case): aatteka@aatteka-MacBookPro:~/Git/ovs$ ps -Af | grep ovsdb aatteka 20116 985 0 17:14 ? 00:00:00 ovsdb-server: monitoring pid 20117 (healthy) aatteka 20117 20116 0 17:14 ? 00:00:00 ovsdb-server /tmp/conf.db -vconsole:emer -vsyslog:err -vfile:info --remote=punix:/var/run/openvswitch/db.sock --private-key=db:Open_vSwitch,SSL,private_key --certificate=db:Open_vSwitch,SSL,certificate --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert --no-chdir --log-file=/var/log/openvswitch/ovsdb-server.log --pidfile=/var/run/openvswitch/ovsdb-server.pid --detach --monitor --user=aatteka 1. However, before being able to do that I had to: sudo chown aatteka:aatteka /var/run/openvswitch. I think you need to get right permissions for this directory 2. Should the monitor process still be run as root? There is a precedent that dnsmasq does that. There are some pros and cons of implementing it that way if the child process on restart would need to acquire certain resources.
On Mon, Sep 7, 2015 at 5:39 PM, Ansis Atteka <ansisatteka@gmail.com> wrote: > > > On 3 September 2015 at 16:33, Andy Zhou <azhou@nicira.com> wrote: >> >> Allow daemon running as root to accept --user option, that accepts >> "user:group" string as input. Performs sanity check on the input, >> and store the converted uid and gid. >> >> daemon_become_new_user() needs to be called to make the actual >> switch. >> >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> >> --- >> lib/daemon-unix.c | 71 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> lib/daemon.h | 27 +++++++++++++++------ >> 2 files changed, 91 insertions(+), 7 deletions(-) >> >> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >> index d52ac2d..44eb800 100644 >> --- a/lib/daemon-unix.c >> +++ b/lib/daemon-unix.c >> @@ -20,6 +20,7 @@ >> #include <errno.h> >> #include <fcntl.h> >> #include <grp.h> >> +#include <pwd.h> >> #include <signal.h> >> #include <stdlib.h> >> #include <string.h> >> @@ -693,6 +694,76 @@ should_service_stop(void) >> return false; >> } >> >> +void daemon_set_new_user(const char *user_spec) >> +{ >> + char *pos = strchr(user_spec, ':'); >> + >> + uid = getuid(); >> + gid = getgid(); >> + >> + if (gid || uid) { >> + VLOG_FATAL("%s: only root can use --user option", pidfile); >> + } >> + >> + user_spec += strspn(user_spec, " \t\r\n"); >> + int len = pos ? pos - user_spec : strlen(user_spec); >> + struct passwd pwd, *res; >> + char buf[4096]; >> + >> + if (len) { >> + user = xzalloc(len + 1); >> + strncpy(user, user_spec, len); >> + >> + if (getpwnam_r(user, &pwd, buf, 4096, &res)) { > > instead of using 4096 I would use "sizeof buf" here and in other places. > > However, from where did you get this "4096" number in the first place? I see > that in the GETPWNAM man page the documented way to get the expected buffer > size is: sysconf(_SC_GETPW_R_SIZE_MAX); Perhaps there is a reason you are > not using that approach? Using sysconf() call will be better, Will fix the next version. > > >> >> + VLOG_FATAL("%s: Invalid --user option %s (no such user %s)", >> + pidfile, user_spec, user); >> + } >> + } else { >> + /* User is not specified, use current user. */ >> + if (getpwuid_r(uid, &pwd, buf, 4096, &res)) { >> + VLOG_FATAL("%s: Invalid --user option %s (failed to lookup " >> + "current user with uid %d)", pidfile, user_spec, >> uid); >> + } >> + user = strdup(pwd.pw_name); >> + } >> + >> + uid = pwd.pw_uid; >> + gid = pwd.pw_gid; >> + >> + if (pos) { >> + char *grpstr = pos + 1; >> + grpstr += strspn(grpstr, " \t\r\n"); >> + >> + if (*grpstr) { >> + struct group grp, *res; >> + >> + if(getgrnam_r(grpstr, &grp, buf, 4096, &res)) { >> + VLOG_FATAL("%s: Invalid --user option %s (unknown group >> %s)", >> + pidfile, user_spec, grpstr); >> + } >> + >> + if(gid != grp.gr_gid) { >> + char **mem; >> + >> + for(mem = grp.gr_mem; *mem; ++mem) { >> + if (!strcmp(*mem, user)) { >> + break; >> + } >> + } >> + >> + if (!*mem) { >> + VLOG_FATAL("%s: Invalid --user option %s (user %s is >> " >> + "not in group %s)", pidfile, user_spec, >> + user, grpstr); >> + } >> + gid = grp.gr_gid; >> + } >> + } >> + } >> + >> + switch_to_new_user = true; >> +} >> + >> void >> daemon_become_new_user(void) >> { >> diff --git a/lib/daemon.h b/lib/daemon.h >> index fb97cde..4b25f46 100644 >> --- a/lib/daemon.h >> +++ b/lib/daemon.h >> @@ -42,14 +42,16 @@ >> OPT_NO_CHDIR, \ >> OPT_OVERWRITE_PIDFILE, \ >> OPT_PIDFILE, \ >> - OPT_MONITOR >> + OPT_MONITOR, \ >> + OPT_USER_GROUP >> >> -#define DAEMON_LONG_OPTIONS \ >> - {"detach", no_argument, NULL, OPT_DETACH}, \ >> - {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, \ >> - {"pidfile", optional_argument, NULL, OPT_PIDFILE}, \ >> +#define DAEMON_LONG_OPTIONS >> \ >> + {"detach", no_argument, NULL, OPT_DETACH}, >> \ >> + {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, >> \ >> + {"pidfile", optional_argument, NULL, OPT_PIDFILE}, >> \ >> {"overwrite-pidfile", no_argument, NULL, OPT_OVERWRITE_PIDFILE}, >> \ >> - {"monitor", no_argument, NULL, OPT_MONITOR} >> + {"monitor", no_argument, NULL, OPT_MONITOR}, >> \ >> + {"user", required_argument, NULL, OPT_USER_GROUP} >> >> #define DAEMON_OPTION_HANDLERS \ >> case OPT_DETACH: \ >> @@ -70,6 +72,10 @@ >> \ >> case OPT_MONITOR: \ >> daemon_set_monitor(); \ >> + break; \ >> + \ >> + case OPT_USER_GROUP: \ >> + daemon_set_new_user(optarg); \ >> break; >> >> void set_detach(void); >> @@ -77,6 +83,7 @@ void daemon_set_monitor(void); >> void set_no_chdir(void); >> void ignore_existing_pidfile(void); >> void daemon_become_new_user(void); >> +void daemon_set_new_user(const char *); >> pid_t read_pidfile(const char *name); >> #else >> #define DAEMON_OPTION_ENUMS \ >> @@ -85,7 +92,7 @@ pid_t read_pidfile(const char *name); >> OPT_PIDFILE, \ >> OPT_PIPE_HANDLE, \ >> OPT_SERVICE, \ >> - OPT_SERVICE_MONITOR >> + OPT_SERVICE_MONITOR \ >> >> #define DAEMON_LONG_OPTIONS >> \ >> {"detach", no_argument, NULL, OPT_DETACH}, >> \ >> @@ -120,6 +127,12 @@ void control_handler(DWORD request); >> void set_pipe_handle(const char *pipe_handle); >> >> static inline void >> +daemon_set_new_user(const char *) >> +{ >> + /* Not implemented. */ >> +} >> + >> +static inline void >> daemon_become_new_user(void) >> { >> /* Not implemented. */ >> -- >> > I was able to downgrade ovsdb-server to a non-root user (aatteka in this > case): > > aatteka@aatteka-MacBookPro:~/Git/ovs$ ps -Af | grep ovsdb > aatteka 20116 985 0 17:14 ? 00:00:00 ovsdb-server: monitoring pid > 20117 (healthy) > aatteka 20117 20116 0 17:14 ? 00:00:00 ovsdb-server /tmp/conf.db > -vconsole:emer -vsyslog:err -vfile:info > --remote=punix:/var/run/openvswitch/db.sock > --private-key=db:Open_vSwitch,SSL,private_key > --certificate=db:Open_vSwitch,SSL,certificate > --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert --no-chdir > --log-file=/var/log/openvswitch/ovsdb-server.log > --pidfile=/var/run/openvswitch/ovsdb-server.pid --detach --monitor > --user=aatteka > Thanks for testing it! I am not user how to write a unit test for this feature. > > 1. However, before being able to do that I had to: sudo chown > aatteka:aatteka /var/run/openvswitch. I think you need to get right > permissions for this directory Many Unix systems, such as ubuntu mounts /var/run to tmpfs. So those directories have to be created with the right user/group and permissions. One possible solution here is for the system to have an OVS group, and have /var/run/oopenvswitch owned by the OVS group with read/write permissions to all OVS groups. We can expand the discussion for when we work on the init script changes. > 2. Should the monitor process still be run as root? There is a precedent > that dnsmasq does that. There are some pros and cons of implementing it that > way if the child process on restart would need to acquire certain resources. > OVSDB restart does not need to root. I can't think of any benefits of running the monitoring process as root, thus it seems better to switch to non-root user as soon as possible, including the monitoring process.
On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote: > Allow daemon running as root to accept --user option, that accepts > "user:group" string as input. Performs sanity check on the input, > and store the converted uid and gid. > > daemon_become_new_user() needs to be called to make the actual > switch. > > Signed-off-by: Andy Zhou <azhou@nicira.com> Thanks for implementing this. There should be a new-line after void here: > +void daemon_set_new_user(const char *user_spec) > +{ I think the ability to use setuid() and friends only depends on having uid 0, not gid 0: > + uid = getuid(); > + gid = getgid(); > + > + if (gid || uid) { > + VLOG_FATAL("%s: only root can use --user option", pidfile); > + } Here in daemon_set_new_user() I would use xmemdup0() instead: > + if (len) { > + user = xzalloc(len + 1); > + strncpy(user, user_spec, len); and this should be xstrdup(): > + user = strdup(pwd.pw_name); > + } I think that the parsing in daemon_set_new_user() assumes that white space, if present, will precede ':'. If not, then 'len' will end up negative, which looks bad to me. I think I'd just not bother worrying about white space in the parameter. I am not sure that it is valuable to check that the user belongs to the specified group. I don't think that other software tends to perform such a check; I don't see one in Apache, for example. I might have other comments when I look at the final patch.
On Tue, Sep 08, 2015 at 05:29:24PM -0700, Ben Pfaff wrote: > On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote: > > Allow daemon running as root to accept --user option, that accepts > > "user:group" string as input. Performs sanity check on the input, > > and store the converted uid and gid. > > > > daemon_become_new_user() needs to be called to make the actual > > switch. > > > > Signed-off-by: Andy Zhou <azhou@nicira.com> > > I might have other comments when I look at the final patch. One more concern. I believe that this series of patches makes all daemons accept --user, but only ovsdb-server actually implements it and the others just treat it as a no-op. I think that this is a bad idea: a server should only accept --user if it implements it. Thanks, Ben.
On 8 September 2015 at 13:09, Andy Zhou <azhou@nicira.com> wrote: > On Mon, Sep 7, 2015 at 5:39 PM, Ansis Atteka <ansisatteka@gmail.com> > wrote: > > > > > > On 3 September 2015 at 16:33, Andy Zhou <azhou@nicira.com> wrote: > >> > >> Allow daemon running as root to accept --user option, that accepts > >> "user:group" string as input. Performs sanity check on the input, > >> and store the converted uid and gid. > >> > >> daemon_become_new_user() needs to be called to make the actual > >> switch. > >> > >> > >> Signed-off-by: Andy Zhou <azhou@nicira.com> > >> --- > >> lib/daemon-unix.c | 71 > >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> lib/daemon.h | 27 +++++++++++++++------ > >> 2 files changed, 91 insertions(+), 7 deletions(-) > >> > >> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > >> index d52ac2d..44eb800 100644 > >> --- a/lib/daemon-unix.c > >> +++ b/lib/daemon-unix.c > >> @@ -20,6 +20,7 @@ > >> #include <errno.h> > >> #include <fcntl.h> > >> #include <grp.h> > >> +#include <pwd.h> > >> #include <signal.h> > >> #include <stdlib.h> > >> #include <string.h> > >> @@ -693,6 +694,76 @@ should_service_stop(void) > >> return false; > >> } > >> > >> +void daemon_set_new_user(const char *user_spec) > >> +{ > >> + char *pos = strchr(user_spec, ':'); > >> + > >> + uid = getuid(); > >> + gid = getgid(); > >> + > >> + if (gid || uid) { > >> + VLOG_FATAL("%s: only root can use --user option", pidfile); > >> + } > >> + > >> + user_spec += strspn(user_spec, " \t\r\n"); > >> + int len = pos ? pos - user_spec : strlen(user_spec); > >> + struct passwd pwd, *res; > >> + char buf[4096]; > >> + > >> + if (len) { > >> + user = xzalloc(len + 1); > >> + strncpy(user, user_spec, len); > >> + > >> + if (getpwnam_r(user, &pwd, buf, 4096, &res)) { > > > > instead of using 4096 I would use "sizeof buf" here and in other places. > > > > However, from where did you get this "4096" number in the first place? I > see > > that in the GETPWNAM man page the documented way to get the expected > buffer > > size is: sysconf(_SC_GETPW_R_SIZE_MAX); Perhaps there is a reason you are > > not using that approach? > Using sysconf() call will be better, Will fix the next version. > > > > > >> > >> + VLOG_FATAL("%s: Invalid --user option %s (no such user > %s)", > >> + pidfile, user_spec, user); > >> + } > >> + } else { > >> + /* User is not specified, use current user. */ > >> + if (getpwuid_r(uid, &pwd, buf, 4096, &res)) { > >> + VLOG_FATAL("%s: Invalid --user option %s (failed to lookup > " > >> + "current user with uid %d)", pidfile, user_spec, > >> uid); > >> + } > >> + user = strdup(pwd.pw_name); > >> + } > >> + > >> + uid = pwd.pw_uid; > >> + gid = pwd.pw_gid; > >> + > >> + if (pos) { > >> + char *grpstr = pos + 1; > >> + grpstr += strspn(grpstr, " \t\r\n"); > >> + > >> + if (*grpstr) { > >> + struct group grp, *res; > >> + > >> + if(getgrnam_r(grpstr, &grp, buf, 4096, &res)) { > >> + VLOG_FATAL("%s: Invalid --user option %s (unknown group > >> %s)", > >> + pidfile, user_spec, grpstr); > >> + } > >> + > >> + if(gid != grp.gr_gid) { > >> + char **mem; > >> + > >> + for(mem = grp.gr_mem; *mem; ++mem) { > >> + if (!strcmp(*mem, user)) { > >> + break; > >> + } > >> + } > >> + > >> + if (!*mem) { > >> + VLOG_FATAL("%s: Invalid --user option %s (user %s > is > >> " > >> + "not in group %s)", pidfile, user_spec, > >> + user, grpstr); > >> + } > >> + gid = grp.gr_gid; > >> + } > >> + } > >> + } > >> + > >> + switch_to_new_user = true; > >> +} > >> + > >> void > >> daemon_become_new_user(void) > >> { > >> diff --git a/lib/daemon.h b/lib/daemon.h > >> index fb97cde..4b25f46 100644 > >> --- a/lib/daemon.h > >> +++ b/lib/daemon.h > >> @@ -42,14 +42,16 @@ > >> OPT_NO_CHDIR, \ > >> OPT_OVERWRITE_PIDFILE, \ > >> OPT_PIDFILE, \ > >> - OPT_MONITOR > >> + OPT_MONITOR, \ > >> + OPT_USER_GROUP > >> > >> -#define DAEMON_LONG_OPTIONS > \ > >> - {"detach", no_argument, NULL, OPT_DETACH}, > \ > >> - {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, > \ > >> - {"pidfile", optional_argument, NULL, OPT_PIDFILE}, > \ > >> +#define DAEMON_LONG_OPTIONS > >> \ > >> + {"detach", no_argument, NULL, OPT_DETACH}, > >> \ > >> + {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, > >> \ > >> + {"pidfile", optional_argument, NULL, OPT_PIDFILE}, > >> \ > >> {"overwrite-pidfile", no_argument, NULL, > OPT_OVERWRITE_PIDFILE}, > >> \ > >> - {"monitor", no_argument, NULL, OPT_MONITOR} > >> + {"monitor", no_argument, NULL, OPT_MONITOR}, > >> \ > >> + {"user", required_argument, NULL, OPT_USER_GROUP} > >> > >> #define DAEMON_OPTION_HANDLERS \ > >> case OPT_DETACH: \ > >> @@ -70,6 +72,10 @@ > >> \ > >> case OPT_MONITOR: \ > >> daemon_set_monitor(); \ > >> + break; \ > >> + \ > >> + case OPT_USER_GROUP: \ > >> + daemon_set_new_user(optarg); \ > >> break; > >> > >> void set_detach(void); > >> @@ -77,6 +83,7 @@ void daemon_set_monitor(void); > >> void set_no_chdir(void); > >> void ignore_existing_pidfile(void); > >> void daemon_become_new_user(void); > >> +void daemon_set_new_user(const char *); > >> pid_t read_pidfile(const char *name); > >> #else > >> #define DAEMON_OPTION_ENUMS \ > >> @@ -85,7 +92,7 @@ pid_t read_pidfile(const char *name); > >> OPT_PIDFILE, \ > >> OPT_PIPE_HANDLE, \ > >> OPT_SERVICE, \ > >> - OPT_SERVICE_MONITOR > >> + OPT_SERVICE_MONITOR \ > >> > >> #define DAEMON_LONG_OPTIONS > >> \ > >> {"detach", no_argument, NULL, OPT_DETACH}, > >> \ > >> @@ -120,6 +127,12 @@ void control_handler(DWORD request); > >> void set_pipe_handle(const char *pipe_handle); > >> > >> static inline void > >> +daemon_set_new_user(const char *) > >> +{ > >> + /* Not implemented. */ > >> +} > >> + > >> +static inline void > >> daemon_become_new_user(void) > >> { > >> /* Not implemented. */ > >> -- > >> > > I was able to downgrade ovsdb-server to a non-root user (aatteka in this > > case): > > > > aatteka@aatteka-MacBookPro:~/Git/ovs$ ps -Af | grep ovsdb > > aatteka 20116 985 0 17:14 ? 00:00:00 ovsdb-server: monitoring > pid > > 20117 (healthy) > > aatteka 20117 20116 0 17:14 ? 00:00:00 ovsdb-server /tmp/conf.db > > -vconsole:emer -vsyslog:err -vfile:info > > --remote=punix:/var/run/openvswitch/db.sock > > --private-key=db:Open_vSwitch,SSL,private_key > > --certificate=db:Open_vSwitch,SSL,certificate > > --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert --no-chdir > > --log-file=/var/log/openvswitch/ovsdb-server.log > > --pidfile=/var/run/openvswitch/ovsdb-server.pid --detach --monitor > > --user=aatteka > > > Thanks for testing it! I am not user how to write a unit test for this > feature. > > > > 1. However, before being able to do that I had to: sudo chown > > aatteka:aatteka /var/run/openvswitch. I think you need to get right > > permissions for this directory > Many Unix systems, such as ubuntu mounts /var/run to tmpfs. So those > directories > have to be created with the right user/group and permissions. One possible > solution here is for the system to have an OVS group, and have > /var/run/oopenvswitch owned > by the OVS group with read/write permissions to all OVS groups. We can > expand the discussion > for when we work on the init script changes. > > > 2. Should the monitor process still be run as root? There is a precedent > > that dnsmasq does that. There are some pros and cons of implementing it > that > > way if the child process on restart would need to acquire certain > resources. > > > OVSDB restart does not need to root. I can't think of any benefits of > running the monitoring process > as root, thus it seems better to switch to non-root user as soon as > possible, including the monitoring > process. > Sorry, I did not use the right word. Instead of "restart" I meant "crash". The scenario I thought about is that if a child process died then the monitor process would have to start the next child process as root so that next child process would be able to acquire privileged resources if it has to. I understand that you don't needs this for ovsdb-server through.
On Tue, Sep 8, 2015 at 5:37 PM, Ben Pfaff <blp@nicira.com> wrote: > On Tue, Sep 08, 2015 at 05:29:24PM -0700, Ben Pfaff wrote: >> On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote: >> > Allow daemon running as root to accept --user option, that accepts >> > "user:group" string as input. Performs sanity check on the input, >> > and store the converted uid and gid. >> > >> > daemon_become_new_user() needs to be called to make the actual >> > switch. >> > >> > Signed-off-by: Andy Zhou <azhou@nicira.com> >> >> I might have other comments when I look at the final patch. > > One more concern. I believe that this series of patches makes all > daemons accept --user, but only ovsdb-server actually implements it and > the others just treat it as a no-op. I think that this is a bad idea: a > server should only accept --user if it implements it. It seems having all daemons accept --user would be a useful feature in the long run. OVSDB happens to be the easiest to add support for since it does not really root privilege to run. Sure, I will work on a way to block this option (and map page) for other daemons.
On Tue, Sep 8, 2015 at 5:29 PM, Ben Pfaff <blp@nicira.com> wrote: > On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote: >> Allow daemon running as root to accept --user option, that accepts >> "user:group" string as input. Performs sanity check on the input, >> and store the converted uid and gid. >> >> daemon_become_new_user() needs to be called to make the actual >> switch. >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> > > Thanks for implementing this. > > There should be a new-line after void here: >> +void daemon_set_new_user(const char *user_spec) >> +{ > oops. Will fix. > I think the ability to use setuid() and friends only depends on having > uid 0, not gid 0: >> + uid = getuid(); >> + gid = getgid(); >> + >> + if (gid || uid) { >> + VLOG_FATAL("%s: only root can use --user option", pidfile); >> + } > You are right. I will fix this and check for effective uid Instead. > Here in daemon_set_new_user() I would use xmemdup0() instead: >> + if (len) { >> + user = xzalloc(len + 1); >> + strncpy(user, user_spec, len); > > and this should be xstrdup(): >> + user = strdup(pwd.pw_name); >> + } > Thanks for pointing out. Will change. > I think that the parsing in daemon_set_new_user() assumes that white > space, if present, will precede ':'. If not, then 'len' will end up > negative, which looks bad to me. I think I'd just not bother worrying > about white space in the parameter. > Sure, I can drop the check. Why would len end up being negative? If pos is set, meaning : is part of the string, then strspn should not look beyond pos, right? > I am not sure that it is valuable to check that the user belongs to the > specified group. I don't think that other software tends to perform > such a check; I don't see one in Apache, for example. > I got that idea from the "daemon" program. Would it otherwise be a security risk of creating an illegal combination of user/group? > I might have other comments when I look at the final patch.
On Wed, Sep 09, 2015 at 12:06:56AM -0700, Andy Zhou wrote: > On Tue, Sep 8, 2015 at 5:29 PM, Ben Pfaff <blp@nicira.com> wrote: > > On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote: > >> Allow daemon running as root to accept --user option, that accepts > >> "user:group" string as input. Performs sanity check on the input, > >> and store the converted uid and gid. > >> > >> daemon_become_new_user() needs to be called to make the actual > >> switch. > >> > >> Signed-off-by: Andy Zhou <azhou@nicira.com> > > > I think that the parsing in daemon_set_new_user() assumes that white > > space, if present, will precede ':'. If not, then 'len' will end up > > negative, which looks bad to me. I think I'd just not bother worrying > > about white space in the parameter. > > > Sure, I can drop the check. Why would len end up being negative? If > pos is set, meaning : is part of the string, then strspn should not > look beyond pos, right? Oops, you're right, sorry. > > I am not sure that it is valuable to check that the user belongs to the > > specified group. I don't think that other software tends to perform > > such a check; I don't see one in Apache, for example. > > > I got that idea from the "daemon" program. Would it otherwise be a > security risk of creating an illegal combination of user/group? I didn't know there was precedent for such a check; I hadn't ever seen one before. Leave it in, then, and we can reconsider if it happens to cause trouble for someone.
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index d52ac2d..44eb800 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -20,6 +20,7 @@ #include <errno.h> #include <fcntl.h> #include <grp.h> +#include <pwd.h> #include <signal.h> #include <stdlib.h> #include <string.h> @@ -693,6 +694,76 @@ should_service_stop(void) return false; } +void daemon_set_new_user(const char *user_spec) +{ + char *pos = strchr(user_spec, ':'); + + uid = getuid(); + gid = getgid(); + + if (gid || uid) { + VLOG_FATAL("%s: only root can use --user option", pidfile); + } + + user_spec += strspn(user_spec, " \t\r\n"); + int len = pos ? pos - user_spec : strlen(user_spec); + struct passwd pwd, *res; + char buf[4096]; + + if (len) { + user = xzalloc(len + 1); + strncpy(user, user_spec, len); + + if (getpwnam_r(user, &pwd, buf, 4096, &res)) { + VLOG_FATAL("%s: Invalid --user option %s (no such user %s)", + pidfile, user_spec, user); + } + } else { + /* User is not specified, use current user. */ + if (getpwuid_r(uid, &pwd, buf, 4096, &res)) { + VLOG_FATAL("%s: Invalid --user option %s (failed to lookup " + "current user with uid %d)", pidfile, user_spec, uid); + } + user = strdup(pwd.pw_name); + } + + uid = pwd.pw_uid; + gid = pwd.pw_gid; + + if (pos) { + char *grpstr = pos + 1; + grpstr += strspn(grpstr, " \t\r\n"); + + if (*grpstr) { + struct group grp, *res; + + if(getgrnam_r(grpstr, &grp, buf, 4096, &res)) { + VLOG_FATAL("%s: Invalid --user option %s (unknown group %s)", + pidfile, user_spec, grpstr); + } + + if(gid != grp.gr_gid) { + char **mem; + + for(mem = grp.gr_mem; *mem; ++mem) { + if (!strcmp(*mem, user)) { + break; + } + } + + if (!*mem) { + VLOG_FATAL("%s: Invalid --user option %s (user %s is " + "not in group %s)", pidfile, user_spec, + user, grpstr); + } + gid = grp.gr_gid; + } + } + } + + switch_to_new_user = true; +} + void daemon_become_new_user(void) { diff --git a/lib/daemon.h b/lib/daemon.h index fb97cde..4b25f46 100644 --- a/lib/daemon.h +++ b/lib/daemon.h @@ -42,14 +42,16 @@ OPT_NO_CHDIR, \ OPT_OVERWRITE_PIDFILE, \ OPT_PIDFILE, \ - OPT_MONITOR + OPT_MONITOR, \ + OPT_USER_GROUP -#define DAEMON_LONG_OPTIONS \ - {"detach", no_argument, NULL, OPT_DETACH}, \ - {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, \ - {"pidfile", optional_argument, NULL, OPT_PIDFILE}, \ +#define DAEMON_LONG_OPTIONS \ + {"detach", no_argument, NULL, OPT_DETACH}, \ + {"no-chdir", no_argument, NULL, OPT_NO_CHDIR}, \ + {"pidfile", optional_argument, NULL, OPT_PIDFILE}, \ {"overwrite-pidfile", no_argument, NULL, OPT_OVERWRITE_PIDFILE}, \ - {"monitor", no_argument, NULL, OPT_MONITOR} + {"monitor", no_argument, NULL, OPT_MONITOR}, \ + {"user", required_argument, NULL, OPT_USER_GROUP} #define DAEMON_OPTION_HANDLERS \ case OPT_DETACH: \ @@ -70,6 +72,10 @@ \ case OPT_MONITOR: \ daemon_set_monitor(); \ + break; \ + \ + case OPT_USER_GROUP: \ + daemon_set_new_user(optarg); \ break; void set_detach(void); @@ -77,6 +83,7 @@ void daemon_set_monitor(void); void set_no_chdir(void); void ignore_existing_pidfile(void); void daemon_become_new_user(void); +void daemon_set_new_user(const char *); pid_t read_pidfile(const char *name); #else #define DAEMON_OPTION_ENUMS \ @@ -85,7 +92,7 @@ pid_t read_pidfile(const char *name); OPT_PIDFILE, \ OPT_PIPE_HANDLE, \ OPT_SERVICE, \ - OPT_SERVICE_MONITOR + OPT_SERVICE_MONITOR \ #define DAEMON_LONG_OPTIONS \ {"detach", no_argument, NULL, OPT_DETACH}, \ @@ -120,6 +127,12 @@ void control_handler(DWORD request); void set_pipe_handle(const char *pipe_handle); static inline void +daemon_set_new_user(const char *) +{ + /* Not implemented. */ +} + +static inline void daemon_become_new_user(void) { /* Not implemented. */
Allow daemon running as root to accept --user option, that accepts "user:group" string as input. Performs sanity check on the input, and store the converted uid and gid. daemon_become_new_user() needs to be called to make the actual switch. Signed-off-by: Andy Zhou <azhou@nicira.com> --- lib/daemon-unix.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/daemon.h | 27 +++++++++++++++------ 2 files changed, 91 insertions(+), 7 deletions(-)