chroot: improve --userspec and --groups look-up

- Support arbitrary numbers in --groups, consistent with
  what is already done for --userspec
- Avoid look-ups entirely for --groups items with a leading '+'
- Support names that are actually numbers in --groups
- Ignore an empty --groups="" option for consistency with --userspec
- Look up both inside and outside the chroot with inside taking
  precedence.  The look-up outside may load required libraries
  to complete the look-up inside the chroot.  This can happen for
  example with a 32 bit chroot on a 64 bit system, where the
  32 bit NSS plugins within the chroot fail to load.

* src/chroot.c (parse_additional_groups): A new function refactored
from set_addition_groups(), to just do the parsing.  The actual
setgroups() call is separated out for calling from the chroot later.
(main): Call parse_user_spec() and parse_additional_groups()
both outside and inside the chroot for the reasons outlined above.
* tests/misc/chroot-credentials.sh: Ensure arbitrary numeric IDs
can be specified without causing look-up errors.
* NEWS: Mention the improvements.
* THANKS.in: Add Norihiro Kamae who initially reported the issue
with a proposed patch.
Also thanks to Dmitry V. Levin for his diagnosis and sample patch.

1 files changed, 32 insertions, 8 deletions

diff --git a/tests/misc/chroot-credentials.sh b/tests/misc/chroot-credentials.sh
index 2b859d8ac..904696d1c 100755
--- a/tests/misc/chroot-credentials.sh
+++ b/tests/misc/chroot-credentials.sh
@@ -22,7 +22,10 @@ print_ver_ chroot
 
 require_root_
 
-root=$(id -nu 0) || skip_ "Couldn't lookup root username"
+grep '^#define HAVE_SETGROUPS 1' "$CONFIG_HEADER" >/dev/null \
+  && HAVE_SETGROUPS=1
+
+root=$(id -nu 0) || skip_ "Couldn't look up root username"
 
 # Verify that root credentials are kept.
 test $(chroot / whoami) = "$root" || fail=1
@@ -34,20 +37,41 @@ whoami_after_chroot=$(
 )
 test "$whoami_after_chroot" != "$root" || fail=1
 
-# Verify that there are no additional groups.
-id_G_after_chroot=$(
-  chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP \
-    --groups=$NON_ROOT_GROUP / id -G
-)
-test "$id_G_after_chroot" = $NON_ROOT_GROUP || fail=1
+if test "$HAVE_SETGROUPS"; then
+  # Verify that there are no additional groups.
+  id_G_after_chroot=$(
+    chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP \
+      --groups=$NON_ROOT_GROUP / id -G
+  )
+  test "$id_G_after_chroot" = $NON_ROOT_GROUP || fail=1
+fi
 
 # Verify that when specifying only the user name we get the current
 # primary group ID.
 test "$(chroot --userspec=$NON_ROOT_USERNAME / id -g)" = "$(id -g)" \
-    || fail=1
+  || fail=1
 
 # Verify that when specifying only a group we get the current user ID
 test "$(chroot --userspec=:$NON_ROOT_GROUP / id -u)" = "$(id -u)" \
+  || fail=1
+
+# verify that invalid groups are diagnosed
+for g in ' ' ',' '0trail'; do
+  test "$(chroot --groups="$g" / id -G)" && fail=1
+done
+
+if test "$HAVE_SETGROUPS"; then
+  # verify that arbitrary numeric IDs are supported
+  test "$(chroot --userspec=1234:+5678 --groups=' +8765,4321' / id -G)" \
+    || fail=1
+
+  # demonstrate that extraneous commas are supported
+  test "$(chroot --userspec=1234:+5678 --groups=',8765,,4321,' / id -G)" \
+    || fail=1
+
+  # demonstrate that --groups is not cumlative
+  test "$(chroot --groups='invalid ignored' --groups='' / id -G)" \
     || fail=1
+fi
 
 Exit $fail