[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

[Nbd] [RFC PATCH] nbd-server: set supplementary groups by default when changing UID/GID



Before this change, there was no way to clear or change supplementary
groups at all, which is usually required to be done along with changing
UID and GID.  This change introduces a new global config boolean option
"setgroups" and enables it by default.  When this option is set to true,
- "group" option will additionally clear the list of supplementary groups;
- unless "group" option is specified, "user" option will additionally
  change both GID and the list of supplementary groups to those defined
  by the given user name.

Signed-off-by: Dmitry V. Levin <ldv@...1147...>
---
I'm not sure this is the best way to handle the issue of supplementary
groups.  To provide 100% backwards compatibility, "setgroups" should be
not enabled by default, but I doubt the current default to keep the list
of supplementary groups unchanged has any practical sense.  If I had to
implement user switching from scratch, I would add just one option "user"
specifying uid, gid and the list of supplementary groups.

 man/nbd-server.5.in.sgml |   17 +++++++++++++++++
 nbd-server.c             |   15 +++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/man/nbd-server.5.in.sgml b/man/nbd-server.5.in.sgml
index 373e620..6cc2fd6 100644
--- a/man/nbd-server.5.in.sgml
+++ b/man/nbd-server.5.in.sgml
@@ -231,6 +231,23 @@ manpage.1: manpage.sgml
 	</listitem>
       </varlistentry>
       <varlistentry>
+	<term><option>setgroups</option></term>
+        <listitem>
+          <para>
+	    Optional; boolean; default true.
+	  </para>
+	  <para>
+	    Define whether <option>user</option> and <option>group</option>
+	    options will also set the list of supplementary groups.  If this
+	    option is set to true, then <option>group</option> option will
+	    clear the list of supplementary groups, and, if
+	    <option>group</option> is not specified, <option>user</option>
+	    option will change both GID and the list of supplementary groups
+	    to those defined by the given user name.
+	  </para>
+        </listitem>
+      </varlistentry>
+      <varlistentry>
 	<term><option>user</option></term>
         <listitem>
           <para>
diff --git a/nbd-server.c b/nbd-server.c
index 489eeb4..1c21907 100644
--- a/nbd-server.c
+++ b/nbd-server.c
@@ -126,8 +126,6 @@ gchar* config_file_pos;
 gchar* runuser=NULL;
 /** What group we're running as */
 gchar* rungroup=NULL;
-/** global flags */
-int glob_flags=0;
 
 /* Whether we should avoid forking */
 int dontfork = 0;
@@ -183,6 +181,9 @@ int dont_daemonize = 0;
 /** Global flags: */
 #define F_OLDSTYLE 1	  /**< Allow oldstyle (port-based) exports */
 #define F_LIST 2	  /**< Allow clients to list the exports on a server */
+#define F_SETGROUPS 4	  /**< Set supplementary groups of the process */
+int glob_flags = F_SETGROUPS;
+
 GHashTable *children;
 char pidfname[256]; /**< name of our PID file */
 char pidftemplate[256]; /**< template to be used for the filename of the PID file */
@@ -890,6 +891,7 @@ GArray* parse_cfile(gchar* f, bool have_global, GError** e) {
 		{ "port", 	FALSE, PARAM_STRING,	&modernport, 	0 },
 		{ "includedir", FALSE, PARAM_STRING,	&cfdir,		0 },
 		{ "allowlist",  FALSE, PARAM_BOOL,	&glob_flags,	F_LIST },
+		{ "setgroups",  FALSE, PARAM_BOOL,	&glob_flags,	F_SETGROUPS },
 	};
 	PARAM* p=gp;
 	int p_size=sizeof(gp)/sizeof(PARAM);
@@ -2561,6 +2563,8 @@ void dousers(void) {
 		if(setgid(gr->gr_gid)<0) {
 			err("Could not set GID: %m"); 
 		}
+		if ((glob_flags & F_SETGROUPS) && setgroups(0, NULL) < 0)
+			err("setgroups: %m");
 	}
 	if(runuser) {
 		pw=getpwnam(runuser);
@@ -2568,6 +2572,13 @@ void dousers(void) {
 			str = g_strdup_printf("Invalid user name: %s", runuser);
 			err(str);
 		}
+		if ((glob_flags & F_SETGROUPS) && !rungroup) {
+			if (setgid(pw->pw_gid) < 0) {
+				err("Could not set GID: %m");
+			if (initgroups(runuser, pw->pw_gid) < 0)
+				err("initgroups: %m");
+		}
+		}
 		if(setuid(pw->pw_uid)<0) {
 			err("Could not set UID: %m");
 		}
-- 
ldv



Reply to: