diff mbox

[ovs-dev,v3,04/10] lib/damon: add --user option

Message ID 1442271254-27897-5-git-send-email-azhou@nicira.com
State Changes Requested
Headers show

Commit Message

Andy Zhou Sept. 14, 2015, 10:54 p.m. UTC 
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@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(-)

Comments

Ben Pfaff Sept. 18, 2015, 7:44 p.m. UTC | #1 
On Mon, Sep 14, 2015 at 03:54:08PM -0700, Andy Zhou 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@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()

"sparse" doesn't like the variable-length array in
daemon_set_new_user().  I'd use xmalloc() instead.
Ben Pfaff Sept. 18, 2015, 7:45 p.m. UTC | #2 
On Fri, Sep 18, 2015 at 12:44:40PM -0700, Ben Pfaff wrote:
> On Mon, Sep 14, 2015 at 03:54:08PM -0700, Andy Zhou 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@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()
> 
> "sparse" doesn't like the variable-length array in
> daemon_set_new_user().  I'd use xmalloc() instead.

Also, s/damon/daemon/ in the commit message.
Ansis Atteka Sept. 20, 2015, 12:14 a.m. UTC | #3 
On Mon, Sep 14, 2015 at 3:54 PM, Andy Zhou <azhou@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@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
alex wang Oct. 12, 2015, 5:33 a.m. UTC | #4 
Hey Andy,

Sorry for this late post, just very interested in the feature
implemented in this series, one question below,

+
> +    user_spec += strspn(user_spec, " \t\r\n");
>


In which circumstance will the option value be prefixed
by "\t\r\n"?

Is this some common parsing practice?

Thanks,
Alex Wang,
Andy Zhou Oct. 12, 2015, 6:25 p.m. UTC | #5 
On Sun, Oct 11, 2015 at 10:33 PM, ALeX Wang <ee07b291@gmail.com> wrote:
> Hey Andy,
>
> Sorry for this late post, just very interested in the feature
> implemented in this series, one question below,
>
>> +
>> +    user_spec += strspn(user_spec, " \t\r\n");
>
>
>
> In which circumstance will the option value be prefixed
> by "\t\r\n"?
In case the input is provided with shell quotes, and white spaces are
within the quotes.
>
> Is this some common parsing practice?
It is common for filtering user input. May not be so common in command
line parsing.
>
> Thanks,
> Alex Wang,
>
>
> --
> Alex Wang,
> Open vSwitch developer
alex wang Oct. 12, 2015, 8:11 p.m. UTC | #6 
Thx, all make sense,

On 12 October 2015 at 11:25, Andy Zhou <azhou@nicira.com> wrote:

> On Sun, Oct 11, 2015 at 10:33 PM, ALeX Wang <ee07b291@gmail.com> wrote:
> > Hey Andy,
> >
> > Sorry for this late post, just very interested in the feature
> > implemented in this series, one question below,
> >
> >> +
> >> +    user_spec += strspn(user_spec, " \t\r\n");
> >
> >
> >
> > In which circumstance will the option value be prefixed
> > by "\t\r\n"?
> In case the input is provided with shell quotes, and white spaces are
> within the quotes.
> >
> > Is this some common parsing practice?
> It is common for filtering user input. May not be so common in command
> line parsing.
> >
> > Thanks,
> > Alex Wang,
> >
> >
> > --
> > Alex Wang,
> > Open vSwitch developer
>
diff mbox

Patch

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);
     }
 }
+
+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);
+    }
+
+    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)) {
+                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;
+}
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);