diff mbox

[ovs-dev,4/5] lib/daemon: Move the user:group code up one level

Message ID 1450463278-7931-5-git-send-email-aconole@redhat.com
State Changes Requested
Headers show

Commit Message

Aaron Conole Dec. 18, 2015, 6:27 p.m. UTC
It will be useful in the future to be able to set ownership on other
files which Open vSwitch creates. Allowing the specification of such
ownership using the standard user:group notation by the user is
desirable. So move the code which parses that information up one level
to be used from other modules.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/daemon-unix.c | 120 ++++++++++++++++++++++++++++++++++--------------------
 lib/daemon.h      |   1 +
 2 files changed, 77 insertions(+), 44 deletions(-)

Comments

Kevin Traynor Dec. 21, 2015, 6:04 p.m. UTC | #1
> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, December 18, 2015 6:28 PM
> To: dev@openvswitch.org
> Cc: Flavio Leitner; Traynor, Kevin
> Subject: [PATCH 4/5] lib/daemon: Move the user:group code up one level
> 
> It will be useful in the future to be able to set ownership on other
> files which Open vSwitch creates. Allowing the specification of such
> ownership using the standard user:group notation by the user is
> desirable. So move the code which parses that information up one level
> to be used from other modules.

I don't think patches 4-5 are related to 1-3. Having them together might
hold up one or the other?
Aaron Conole Dec. 21, 2015, 7:27 p.m. UTC | #2
"Traynor, Kevin" <kevin.traynor@intel.com> writes:
>> -----Original Message-----
>> From: Aaron Conole [mailto:aconole@redhat.com]
>> Sent: Friday, December 18, 2015 6:28 PM
>> To: dev@openvswitch.org
>> Cc: Flavio Leitner; Traynor, Kevin
>> Subject: [PATCH 4/5] lib/daemon: Move the user:group code up one level
>> 
>> It will be useful in the future to be able to set ownership on other
>> files which Open vSwitch creates. Allowing the specification of such
>> ownership using the standard user:group notation by the user is
>> desirable. So move the code which parses that information up one level
>> to be used from other modules.
>
> I don't think patches 4-5 are related to 1-3. Having them together might
> hold up one or the other? 

Okay, I'll cut them. They're definitely not related to the core of the
patch, so it probably makes sense to submit them after patches 1-3 are
finished up and accepted.

Thanks for the review, Kevin!

-Aaron
diff mbox

Patch

diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
index 182f76b..62afceb 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -926,72 +926,68 @@  enlarge_buffer(char **buf, size_t *sizep)
     return false;
 }
 
-/* Parse and sanity check user_spec.
- *
- * If successful, set global variables 'uid' and 'gid'
- * with the parsed results. Global variable 'user'
- * will be pointing to a string that stores the name
- * of the user to be switched into.
- *
- * Also set 'switch_to_new_user' to true, The actual
- * user switching is done as soon as daemonize_start()
- * is called. I/O access before calling daemonize_start()
- * will still be with root's credential.  */
-void
-daemon_set_new_user(const char *user_spec)
+int
+get_owners_from_str(const char *user_spec, uid_t *uid, char **user,
+                    gid_t *gid, bool validate_user_group)
 {
     char *pos = strchr(user_spec, ':');
     size_t init_bufsize, bufsize;
 
     init_bufsize = get_sysconf_buffer_size();
-    uid = getuid();
-    gid = getgid();
-
-    if (geteuid() || uid) {
-        VLOG_FATAL("%s: only root can use --user option", pidfile);
-    }
-
     user_spec += strspn(user_spec, " \t\r\n");
+
     size_t len = pos ? pos - user_spec : strlen(user_spec);
-    char *buf;
+
+    char *buf = NULL;
     struct passwd pwd, *res;
     int e;
 
     bufsize = init_bufsize;
     buf = xmalloc(bufsize);
+    char *user_search = NULL;
     if (len) {
-        user = xmemdup0(user_spec, len);
-
-        while ((e = getpwnam_r(user, &pwd, buf, bufsize, &res)) == ERANGE) {
+        user_search = xmemdup0(user_spec, len);
+        while ((e = getpwnam_r(user_search, &pwd, buf, bufsize, &res)) == ERANGE) {
             if (!enlarge_buffer(&buf, &bufsize)) {
                 break;
             }
         }
 
         if (e != 0) {
-            VLOG_FATAL("%s: Failed to retrive user %s's uid (%s), aborting.",
-                       pidfile, user, ovs_strerror(e));
+            VLOG_ERR("%s: Failed to retrive user %s's uid (%s), aborting.",
+                     pidfile, user_search, ovs_strerror(e));
+            goto release;
         }
     } else {
         /* User name is not specified, use current user.  */
-        while ((e = getpwuid_r(uid, &pwd, buf, bufsize, &res)) == ERANGE) {
+        while ((e = getpwuid_r(getuid(), &pwd, buf, bufsize, &res)) == ERANGE) {
             if (!enlarge_buffer(&buf, &bufsize)) {
                 break;
             }
         }
 
         if (e != 0) {
-            VLOG_FATAL("%s: Failed to retrive current user's name "
-                       "(%s), aborting.", pidfile, ovs_strerror(e));
+            VLOG_ERR("%s: Failed to retrive current user's name "
+                     "(%s), aborting.", pidfile, ovs_strerror(e));
+            goto release;
         }
-        user = xstrdup(pwd.pw_name);
+        user_search = xstrdup(pwd.pw_name);
     }
 
-    uid = pwd.pw_uid;
-    gid = pwd.pw_gid;
+    if (user)
+        *user = user_search;
+
+    if (uid)
+        *uid = pwd.pw_uid;
+
+    if (gid)
+        *gid = pwd.pw_gid;
+
     free(buf);
+    buf = NULL;
 
     if (pos) {
+        gid_t tmpgid = pwd.pw_gid;
         char *grpstr = pos + 1;
         grpstr += strspn(grpstr, " \t\r\n");
 
@@ -1008,30 +1004,66 @@  daemon_set_new_user(const char *user_spec)
             }
 
             if (e) {
-                VLOG_FATAL("%s: Failed to get group entry for %s, "
-                           "(%s), aborting.", pidfile, grpstr,
-                           ovs_strerror(e));
+                VLOG_ERR("%s: Failed to get group entry for %s, "
+                         "(%s), aborting.", pidfile, grpstr,
+                         ovs_strerror(e));
+                goto release;
             }
 
-            if (gid != grp.gr_gid) {
+            if (tmpgid != grp.gr_gid) {
                 char **mem;
 
                 for (mem = grp.gr_mem; *mem; ++mem) {
-                    if (!strcmp(*mem, user)) {
+                    if (!strcmp(*mem, user_search)) {
                         break;
                     }
                 }
 
-                if (!*mem) {
-                    VLOG_FATAL("%s: Invalid --user option %s (user %s is "
-                               "not in group %s), aborting.", pidfile,
-                               user_spec, user, grpstr);
+                if (!*mem && validate_user_group) {
+                    VLOG_ERR("%s: Invalid user str %s (user %s is "
+                             "not in group %s).", pidfile, user_spec,
+                             user_search, grpstr);
+                    e = EINVAL;
+                    goto release;
                 }
-                gid = grp.gr_gid;
+                if (gid)
+                    *gid = grp.gr_gid;
             }
-            free(buf);
         }
     }
 
-    switch_user = true;
+ release:
+    free(buf);
+    if(e) {
+        free(user_search);
+    }
+    return e;
+}
+
+/* Parse and sanity check user_spec.
+ *
+ * If successful, set global variables 'uid' and 'gid'
+ * with the parsed results. Global variable 'user'
+ * will be pointing to a string that stores the name
+ * of the user to be switched into.
+ *
+ * Also set 'switch_to_new_user' to true, The actual
+ * user switching is done as soon as daemonize_start()
+ * is called. I/O access before calling daemonize_start()
+ * will still be with root's credential.  */
+void
+daemon_set_new_user(const char *user_spec)
+{
+    uid = getuid();
+    gid = getgid();
+
+    if (geteuid() || uid) {
+        VLOG_FATAL("%s: only root can use --user option", pidfile);
+    }
+
+    if (!get_owners_from_str(user_spec, &uid, &user, &gid, true)) {
+        switch_user = true;
+    } else {
+        VLOG_FATAL("Failed --user with option %s. aborting.", user_spec);
+    }
 }
diff --git a/lib/daemon.h b/lib/daemon.h
index 4990415..8742aa8 100644
--- a/lib/daemon.h
+++ b/lib/daemon.h
@@ -83,6 +83,7 @@  void daemon_set_monitor(void);
 void set_no_chdir(void);
 void ignore_existing_pidfile(void);
 pid_t read_pidfile(const char *name);
+int get_owners_from_str(const char *user_spec, uid_t *uid, char **user, gid_t *gid, bool validate_user_group);
 #else
 #define DAEMON_OPTION_ENUMS                    \
     OPT_DETACH,                                \