[bug#74962,v3,4/5] etc/guix-install.sh: Remove 'which' commands from requirements.

  * etc/guix-install.sh (REQUIRE): Remove "which".  Add "nologin".
(sys_create_build_user): Use 'type' instead of 'which'.

Fixes: <https://issues.guix.gnu.org/74952>
Reported-by: Simon Josefsson <simon@josefsson.org>
Change-Id: I0675716bab3fc22d3289ee7af2cb0ab33a1cee71
---

Changes in v3:
 - New.

 etc/guix-install.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> * etc/guix-install.sh (REQUIRE): Remove "which".  Add "nologin".
> (sys_create_build_user): Use 'type' instead of 'which'.
>
> Fixes: <https://issues.guix.gnu.org/74952>
> Reported-by: Simon Josefsson <simon@josefsson.org>
> Change-Id: I0675716bab3fc22d3289ee7af2cb0ab33a1cee71

LGTM.
  

Ludovic Courtès <ludo@gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> * etc/guix-install.sh (REQUIRE): Remove "which".  Add "nologin".
>> (sys_create_build_user): Use 'type' instead of 'which'.
>>
>> Fixes: <https://issues.guix.gnu.org/74952>
>> Reported-by: Simon Josefsson <simon@josefsson.org>
>> Change-Id: I0675716bab3fc22d3289ee7af2cb0ab33a1cee71
>
> LGTM.

Using 'type -P' is not POSIX and neither /bin/dash nor /bin/gash
supports it.  It seems like a GNU bash extension.  Is that okay?

The snippet ends up in the manual as recommendations for users to run on
different operating systems.  We may want to assume GNU bash to favor
it, but I'm not sure if that is really helping users.

If 'type -P' is used, shouldn't that really be 'type -fP' to avoid shell
function expansion?  It isn't all that clear from the man page if -f is
still needed for -P or not:

https://manpages.debian.org/bookworm/bash/bash.1.en.html#type

Even so 'type' uses hashed names, do they survive sub-shell $()
execution?  If type is to be used, maybe this should be:

  $(hash -r nologin && type -Pf nologin)

My suggestion was to use 'command -v nologin' which behaviour is
standard POSIX /bin/sh.  I acknowledge that it has the trouble of
expanding to an alias if the shell had 'nologin' aliases somehow
(unlikely but not impossible).

  $(unalias nologin; command -v nologin)

It seems all of the options (which, type -P, command -v) has another
unwanted property: if 'nologin' is not available in the path, these
commands expand to the empty string, and that empty string gets passed
to 'useradd -s STR -c ...' and the user gets an ugly error message about
'-c' not being a proper shell.

I wonder what all this solves compared to hard-coding "/" as the login
shell for the guixbuild user?

Here is source code for nologin, which we seem to make some effort to
use - is this better than 'false'?

https://github.com/shadow-maint/shadow/blob/master/src/nologin.c

At least I'm happy nobody wants to keep using 'which'.

I am sorry for the rabbit hole :)

/Simon
  

Simon Josefsson <simon@josefsson.org> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>
>>> * etc/guix-install.sh (REQUIRE): Remove "which".  Add "nologin".
>>> (sys_create_build_user): Use 'type' instead of 'which'.
>>>
>>> Fixes: <https://issues.guix.gnu.org/74952>
>>> Reported-by: Simon Josefsson <simon@josefsson.org>
>>> Change-Id: I0675716bab3fc22d3289ee7af2cb0ab33a1cee71
>>
>> LGTM.
>
> Using 'type -P' is not POSIX and neither /bin/dash nor /bin/gash
> supports it.  It seems like a GNU bash extension.  Is that okay?

Oh, not great.  From what you write, I’m not sure what to conclude;
just skip this patch and be done with it?

Thanks!

Ludo’.
  

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Simon Josefsson <simon@josefsson.org> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>>
>>>> * etc/guix-install.sh (REQUIRE): Remove "which".  Add "nologin".
>>>> (sys_create_build_user): Use 'type' instead of 'which'.
>>>>
>>>> Fixes: <https://issues.guix.gnu.org/74952>
>>>> Reported-by: Simon Josefsson <simon@josefsson.org>
>>>> Change-Id: I0675716bab3fc22d3289ee7af2cb0ab33a1cee71
>>>
>>> LGTM.
>>
>> Using 'type -P' is not POSIX and neither /bin/dash nor /bin/gash
>> supports it.  It seems like a GNU bash extension.  Is that okay?
>
> Oh, not great.  From what you write, I’m not sure what to conclude;
> just skip this patch and be done with it?

We currently use other Bash-specific features, so I think it's fine to
embrace the Bash requirement instead of shying away from it.

If we decide that we don't want Bash as a requirement at some point,
we'll have to change a bunch of things; one of them would be to no
longer make use of arrays since POSIX shells don't have them, for
example.
  

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi,
>
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Simon Josefsson <simon@josefsson.org> skribis:
>>
>>> Ludovic Courtès <ludo@gnu.org> writes:
>>>
>>>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>>>
>>>>> * etc/guix-install.sh (REQUIRE): Remove "which".  Add "nologin".
>>>>> (sys_create_build_user): Use 'type' instead of 'which'.
>>>>>
>>>>> Fixes: <https://issues.guix.gnu.org/74952>
>>>>> Reported-by: Simon Josefsson <simon@josefsson.org>
>>>>> Change-Id: I0675716bab3fc22d3289ee7af2cb0ab33a1cee71
>>>>
>>>> LGTM.
>>>
>>> Using 'type -P' is not POSIX and neither /bin/dash nor /bin/gash
>>> supports it.  It seems like a GNU bash extension.  Is that okay?
>>
>> Oh, not great.  From what you write, I’m not sure what to conclude;
>> just skip this patch and be done with it?
>
> We currently use other Bash-specific features, so I think it's fine to
> embrace the Bash requirement instead of shying away from it.
>
> If we decide that we don't want Bash as a requirement at some point,
> we'll have to change a bunch of things; one of them would be to no
> longer make use of arrays since POSIX shells don't have them, for
> example.

There is a difference to use bashisms in code in Guix intended to be run
on bash, and code we have in the manual that is suggested to be used on
other operating system as part of the Guix installation process.

There appears to be no perfect solution here.  I think 'command -v
nologin' is the closest.  Or just keep the code as-is and use 'which',
but that caused my initial problem (lack of 'which').

I'd like to second-guess why we even bother with using "nologin" instead
of simply hard-coding "/bin/false" or why not "/" which I suppose is not
a executable shell on any system.

/Simon
  

Hi Simon,

Simon Josefsson <simon@josefsson.org> writes:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>
>>> * etc/guix-install.sh (REQUIRE): Remove "which".  Add "nologin".
>>> (sys_create_build_user): Use 'type' instead of 'which'.
>>>
>>> Fixes: <https://issues.guix.gnu.org/74952>
>>> Reported-by: Simon Josefsson <simon@josefsson.org>
>>> Change-Id: I0675716bab3fc22d3289ee7af2cb0ab33a1cee71
>>
>> LGTM.
>
> Using 'type -P' is not POSIX and neither /bin/dash nor /bin/gash
> supports it.  It seems like a GNU bash extension.  Is that okay?

I think it's OK, since we currently mandate Bash, but we could use
'command -v the-command > /dev/null' for the same result, which *is*
POSIX, so perhaps we should use that instead.

> The snippet ends up in the manual as recommendations for users to run on
> different operating systems.  We may want to assume GNU bash to favor
> it, but I'm not sure if that is really helping users.
>
> If 'type -P' is used, shouldn't that really be 'type -fP' to avoid shell
> function expansion?  It isn't all that clear from the man page if -f is
> still needed for -P or not:
>
> https://manpages.debian.org/bookworm/bash/bash.1.en.html#type

I think we'd have to use -f if we want to guard against shell functions
being found instead; but maybe then it's simpler and clearer to just use
'command -v' as I mentioned above.

> Even so 'type' uses hashed names, do they survive sub-shell $()
> execution?  If type is to be used, maybe this should be:
>
>   $(hash -r nologin && type -Pf nologin)
>
> My suggestion was to use 'command -v nologin' which behaviour is
> standard POSIX /bin/sh.  I acknowledge that it has the trouble of
> expanding to an alias if the shell had 'nologin' aliases somehow
> (unlikely but not impossible).

I agree; I'll make the change.  Perhaps adjust the other 'type' usages
also (there was only 2).

>   $(unalias nologin; command -v nologin)
>
> It seems all of the options (which, type -P, command -v) has another
> unwanted property: if 'nologin' is not available in the path, these
> commands expand to the empty string, and that empty string gets passed
> to 'useradd -s STR -c ...' and the user gets an ugly error message about
> '-c' not being a proper shell.

Yuck.

> I wonder what all this solves compared to hard-coding "/" as the login
> shell for the guixbuild user?
>
> Here is source code for nologin, which we seem to make some effort to
> use - is this better than 'false'?
>
> https://github.com/shadow-maint/shadow/blob/master/src/nologin.c

It seems marginally better than using 'false' in that it logs something
to syslog when a login is attempted and fail :-).

> At least I'm happy nobody wants to keep using 'which'.
>
> I am sorry for the rabbit hole :)

Thanks for the comments.  I'll send a reworked version.