Re: [PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo

[Carlo Arenas <carenas@gmail.com>]

On Tue, May 3, 2022 at 7:12 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh
> > new file mode 100644
> > index 00000000000..9ebb30fc82b
> > --- /dev/null
> > +++ b/t/lib-sudo.sh
> > @@ -0,0 +1,13 @@
> > +# Helpers for running git commands under sudo.
> > +
> > +# Runs a scriplet passed through stdin under sudo.
> > +run_with_sudo () {
> > +     local ret
> > +     local SH=${1-"$TEST_SHELL_PATH"}
>
> What use do you envisage for this? It would be simpler just to use
> $TEST_SHELL_PATH directly below

Since this function is a ripoff of write_script which has this feature
(except I changed the shell used), I thought it might come handy later
and that getting the first version with a hardcoded value wasn't
elegant enough, but you are right it is not needed, so is gone in the
next RFC.

> > +     local RUN="$HOME/$$.sh"
>
> Can we use a fixed name for the script? That would make things simpler
> especially debugging as one would know what file to look for. Also using
> $TEST_DIRECTORY rather than $HOME would make it clear where the file
> ends up.

Frankly (and speaking by experience because this broke a lot for me),
debugging is painful enough already that having to find the shell
script would be the least of your concerns.

The main reason why it is not hardcoded to a known name is that by
adding a slightly more random name, it makes it slightly more
difficult for someone to try to trick us into running something else
they prepared instead, and the fact that it will be run as root makes
it much more enticing.

Sorry about not using TEST_DIRECTORY to begin with, they are set to
the same value and thought HOME was clearer and shorter, but has been
fixed in the next RFC.

> > +     write_script "$RUN" "$SH"
> > +     sudo "$SH" -c "\"$RUN\""
>
> I think using write_script means we can just do 'sudo "$RUN"'

We could, but by doing it this way we ensure:
* we specifically require sudo to allow running a shell, and that
shell is the one we want it to run (a strict sudo wouldn't do that)
* we obscure a little bit what sudo is running from it and rely more
in the shell, so it won't try to be helpful and block it (running
shell scripts is something a strict sudo might not allow as well)

On that last point I am still debating if the prerequisite should be
enhanced to detect that case and fail, but since it is restrictive
enough already it might not be worth doing now.

> > -# this MUST be always the last test, if used more than once, the next
> > -# test should do a full setup again.
>
> Why is the comment being changed? If you want the shorter version at the
> end of this patch can't we just use that wording in patch 1?

It was my failed attempt to document better how they are used, since
they are tricky enough to use already, but it is no longer needed and
is gone in the next RFC.

> > +test_expect_success SUDO 'cannot access with sudo' '
> > +     (
> > +             # TODO: test_must_fail needs additional functionality
> > +             # 6a67c759489 blocks its use with sudo
> > +             cd root/p &&
> > +             ! sudo git status
> > +     )
> > +'
>
> I think Junio suggested that this should work and showed it was simple
> to make it work. It seems funny that if sudo is started as root it does
> not work.

It is not when sudo is started as root, but when the user runs `sudo
-s` to get a shell and then LATER tries to use git with that shell.

I make a better attempt to explain it in a different thread[1], and is
tied to the documentation patch which might need further improvements
to make clear (hopefully not as extensive as my attempts to do so)

I am instead marking it as a known bug for the next RFC

Carlo

[1] https://lore.kernel.org/git/20220429012438.37o4uaxsrfdu2b6x@carlos-mbp.lan/