Merge pull request #129 from garlick/setgroups

improve setgroups error message and inline documentation

Author: mergify[bot]

scripts/check-root.sh

@@ -10,7 +10,6 @@ sudo -n /bin/true || die "passwordless sudo is required to run privileged tests"
 sudo -n make -C src/libnpfs check TESTS="\
   test_capability.t \
   test_setfsuid.t \
-  test_setgroups.t \
   test_setreuid.t" || die "test failed"
 
 sudo -n make -C src/libdiod check TESTS="\

src/cmd/diod.c

@@ -494,9 +494,14 @@ _service_sigsetup (void)
 }
 
 #if USE_IMPERSONATION_LINUX
-/* Test whether setgroups applies to thread or process.
- * On RHEL6 (2.6.32 based) it applies to threads and we can use it.
- * On Ubuntu 11 (2.6.38 based) it applies to the whole process and we can't.
+/* POSIX setgroups(2) is per process but in Linux the underlying system call
+ * is per-thread and the per-process bit is handled in glibc, so we can use
+ * SYS_setgroups directly in the server thread pool when switching users.
+ * This assumption is tenuous though, so we should quickly check it during
+ * server startup, in case a future kernel update invalidates it.
+ *
+ * If we can't use it, a warning is issued and life goes on with group access
+ * checks based on the user's primary group.
  */
 void *
 _test_setgroups_thread (void *arg)
@@ -529,7 +534,7 @@ _test_setgroups (void)
         err_exit ("pthread_create");
     if ((err = pthread_join (t, NULL)))
         err_exit ("pthread_join");
-    if ((n = getgroups (ngroups_max, sg)) < 0)
+    if ((n = getgroups (0, NULL)) < 0)
         err_exit ("getgroups");
     if (n == 0)
         rc = 1;
@@ -619,8 +624,10 @@ _service_run (srvmode_t mode, int rfdno, int wfdno)
 #if USE_IMPERSONATION_LINUX
         if (_test_setgroups ())
             flags |= SRV_FLAGS_SETGROUPS;
-        else
-            msg ("test_setgroups: groups are per-process (disabling)");
+        else {
+            msg ("warning: supplemental group membership will be ignored."
+                "  Some accesses might be inappropriately denied.");
+        }
 #elif USE_IMPERSONATION_GANESHA
         if (init_ganesha_syscalls() < 0)
             msg ("nfs-ganesha-kmod not loaded: changing user/group will fail");

src/libnpfs/Makefile.am

@@ -51,7 +51,6 @@ TESTS = \
 	test_fidpool.t \
 	test_capability.t \
 	test_setfsuid.t \
-	test_setgroups.t \
 	test_setreuid.t
 
 check_PROGRAMS = $(TESTS)
@@ -71,8 +70,5 @@ test_capability_t_LDADD = $(test_ldadd)
 test_setfsuid_t_SOURCES = test/setfsuid.c
 test_setfsuid_t_LDADD = $(test_ldadd)
 
-test_setgroups_t_SOURCES = test/setgroups.c
-test_setgroups_t_LDADD = $(test_ldadd)
-
 test_setreuid_t_SOURCES = test/setreuid.c
 test_setreuid_t_LDADD = $(test_ldadd)

src/libnpfs/test/setgroups.c

@@ -160,11 +160,23 @@ np_setfsid (Npreq *req, Npuser *u, u32 gid_override)
 			else if (wt->fsuid == 0)
 				wt->privcap = 0; /* trans from 0 clears caps */
 
-			/* Suppl groups need to be part of cred for NFS
-			 * forwarding even with DAC_BYPASS.  However only
-			 * do it if kernel treats sg's per-thread not process.
-			 * Addendum: late model glibc attempts to make this
-			 * per-process, so for now bypass glibc. See issue 53.
+			/* Set the user's supplementary groups (as read from the host
+			 * system's group configuration), but only if it can be
+			 * done safely.
+			 *
+			 * setgroups(2) is per-process (POSIX), which is incompatible
+			 * with the npfs thread pool model.  SYS_setgroups, the
+			 * direct system call as opposed to the libc wrapper, is
+			 * per-thread, but relying on that is fragile.  Therefore,
+			 * require the server to attest that SYS_setgroups is per-
+			 * thread on the server host kernel by setting a flag, e.g.
+			 * based on a test at startup.  If the flag is unset, access
+			 * is determined solely by the user's uid and primary gid.
+			 *
+			 * Even with DAC_BYPASS, the supplementary groups might
+			 * into play if the host file system is NFS, since the
+			 * supplementary groups are still checked on the NFS server.
+			 * Unlike 9P, NFS transmits them over the wire.
 			 */
 			if ((srv->flags & SRV_FLAGS_SETGROUPS)) {
 				if (syscall(SYS_setgroups, u->nsg, u->sg) < 0) {