On 05/17/2014 01:45 AM, Bernhard Voelker wrote: > On 05/16/2014 10:59 PM, Pádraig Brady wrote: > > Thanks for the detailed tests. > >> [[ chroot --user=+5000 / id -G ]] >> before: 0 1 2 3 4 6 10 >> after: src/chroot: failed to get primary group > > While the logic behind may be okay, our users will > probably be confused with the above change in behavior. > I propose to change the error diagnostic to give a hint > that the user must specify the group explicitly when > using a numeric UID. > > >> diff --git a/doc/coreutils.texi b/doc/coreutils.texi >> index 789cd68..bfaae9f 100644 >> --- a/doc/coreutils.texi >> +++ b/doc/coreutils.texi >> @@ -16112,12 +16112,17 @@ By default, @var{command} is run with the same >> credentials >> as the invoking process. >> Use this option to run it as a different @var{user} and/or with a >> different primary @var{group}. >> +If a @var{user} is specified then the supplementary groups >> +are set according to the system defined list for that user, >> +unless overridden with the @option{--groups} option. >> >> @item --groups=@var{groups} >> @opindex --groups >> -Use this option to specify the supplementary @var{groups} to be >> +Use this option to override the supplementary @var{groups} to be >> used by the new process. >> The items in the list (names or numeric IDs) must be separated by commas. >> +Use @samp{--groups=''} to disable the supplementary group lookup > > Didn't we use s/lookup/look-up/ recently, see v8.22-38-ge972be3? > >> diff --git a/src/chroot.c b/src/chroot.c >> index a2debac..a679658 100644 >> --- a/src/chroot.c >> +++ b/src/chroot.c >> @@ -232,12 +250,36 @@ main (int argc, char **argv) >> /* We have to look up users and groups twice. >> - First, outside the chroot to load potentially necessary >> passwd/group >> parsing plugins (e.g. NSS); >> - - Second, inside chroot to redo parsing in case IDs are different. >> */ >> + - Second, inside chroot to redo parsing in case IDs are different. >> + Within chroot lookup is the main justification for having >> + the --user option supported cy the chroot command itself. */ > > s/cy/by/ > > >> - if (gids && setgroups (n_gids, gids) != 0) >> + if ((uid_set (uid) || groups) && setgroups (n_gids, gids) != 0) >> { >> - error (0, errno, _("failed to set additional groups")); >> + error (0, errno, _("failed to %s supplemental groups"), >> + gids ? "set" : "clear"); >> fail = true; >> } >> >> free (in_gids); >> free (out_gids); >> >> - if (gid != (gid_t) -1 && setgid (gid)) >> + if (gid_set (gid) && setgid (gid)) >> { >> error (0, errno, _("failed to set group-ID")); >> fail = true; >> } >> >> - if (uid != (uid_t) -1 && setuid (uid)) >> + if (uid_set (uid) && setuid (uid)) >> { >> error (0, errno, _("failed to set user-ID")); >> fail = true; > > Is there a reason not to exit immediately? > IMHO it looks strange (not to say worrying) to get 3 error messages: > > $ src/chroot --user=+5000:123 / id -G > src/chroot: failed to clear supplemental groups: Operation not permitted > src/chroot: failed to set group-ID: Operation not permitted > src/chroot: failed to set user-ID: Operation not permitted
That was there already, and I was wondering why myself. I'll adjust to exit ASAP lest the user think previous failing actions were optional. Also I agree with your other points and will adjust accordingly. Excellent review. thanks, Pádraig.