[ovs-dev] [PATCH 2/3] lib: Add --user for daemon
Andy Zhou
azhou at nicira.com
Tue Sep 8 20:09:45 UTC 2015
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.
More information about the dev
mailing list