[ovs-dev] [PATCH 2/3] lib: Add --user for daemon
Ansis Atteka
ansisatteka at gmail.com
Wed Sep 9 00:42:05 UTC 2015
On 8 September 2015 at 13:09, Andy Zhou <azhou at nicira.com> wrote:
> On Mon, Sep 7, 2015 at 5:39 PM, Ansis Atteka <ansisatteka at gmail.com>
> wrote:
> >
> >
> > On 3 September 2015 at 16:33, Andy Zhou <azhou at 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 at 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 at 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.
More information about the dev
mailing list