[ovs-dev] [v3 04/10] lib/damon: add --user option
Ansis Atteka
aatteka at nicira.com
Sun Sep 20 00:14:06 UTC 2015
On Mon, Sep 14, 2015 at 3:54 PM, Andy Zhou <azhou at nicira.com> wrote:
> Common implementation for daemons to support the --user option which
> 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 user
> switch.
>
> Signed-off-by: Andy Zhou <azhou at nicira.com>
> ---
> v2 : use sysconf() to get proper buffer size. not hard code it
> v3: only check uid not gid for setuid() permission.
> use xmemdup0() and xstrdup() instead of strncpy() and strdup()
> ---
> lib/daemon-unix.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> lib/daemon.h | 29 +++++++++++++++------
> 2 files changed, 98 insertions(+), 8 deletions(-)
>
> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> index f361165..9942640 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>
> @@ -810,3 +811,79 @@ daemon_become_new_user(bool access_kernel_datapath)
> daemon_switch_user(uid, uid, uid, user);
> }
> }
> +
Would you mind putting a comment here that describes this function? I
would suggest as per CodingStyle file to Document function's input
variables and the global variables that it sets.
> +void
> +daemon_set_new_user(const char *user_spec)
> +{
> + char *pos = strchr(user_spec, ':');
> + int bufsize;
> +
> + uid = getuid();
> + gid = getgid();
> +
> + if (geteuid() || uid) {
> + VLOG_FATAL("%s: only root can use --user option", pidfile);
> + }
> +
> + if ((bufsize = sysconf(_SC_GETPW_R_SIZE_MAX)) == -1) {
> + VLOG_FATAL("%s: Invalid --user option %s (Unknown system password "
> + "configuration)", pidfile, user_spec);
1. is this right log message? I mean it suggests that --user option is
invalid without even looking at it.
2. Note that sysconf(_SC_GETPW_R_SIZE_MAX) may return -1 if there is
no hard limit on the size of the buffer needed to store all the groups
returned. Do you think that this should be treated as fatal condition.
> + }
> +
> + user_spec += strspn(user_spec, " \t\r\n");
> + int len = pos ? pos - user_spec : strlen(user_spec);
> + struct passwd pwd, *res;
> + char buf[bufsize];
> +
> + if (len) {
> + user = xmemdup0(user_spec, len);
> +
> + if (getpwnam_r(user, &pwd, buf, bufsize, &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, bufsize, &res)) {
> + VLOG_FATAL("%s: Invalid --user option %s (failed to lookup "
> + "current user with uid %d)", pidfile, user_spec, uid);
> + }
> + user = xstrdup(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, bufsize, &res)) {
missing space after if
> + VLOG_FATAL("%s: Invalid --user option %s (unknown group %s)",
> + pidfile, user_spec, grpstr);
> + }
> +
> + if(gid != grp.gr_gid) {
missing space after if
> + char **mem;
> +
> + for(mem = grp.gr_mem; *mem; ++mem) {
missing space after for
> + 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;
> +}
> diff --git a/lib/daemon.h b/lib/daemon.h
> index f98ef16..ac5f0e9 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,13 +72,18 @@
> \
> case OPT_MONITOR: \
> daemon_set_monitor(); \
> + break; \
> + \
> + case OPT_USER_GROUP: \
> + daemon_set_new_user(optarg); \
> break;
>
> void set_detach(void);
> void daemon_set_monitor(void);
> void set_no_chdir(void);
> void ignore_existing_pidfile(void);
> -void daemon_become_new_user(bool access_kernel_datapath);
> +void daemon_become_new_user(bool);
> +void daemon_set_new_user(const char * user_spec);
> 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}, \
> @@ -124,6 +131,12 @@ daemon_become_new_user(bool access_kernel_datapath OVS_UNUSED)
> {
> /* Not implemented. */
> }
> +
> +static inline void
> +daemon_set_new_user(const char *user_spec OVS_UNUSED)
> +{
> + /* Not implemented. */
> +}
> #endif /* _WIN32 */
>
> bool get_detach(void);
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list