On Tue, May 10, 2022 at 4:11 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> writes:
>
> > Note that the specific test that documents that after the previous
> > changes, it is no longer possible for root (if obtained through sudo)
> > to NOT add an exception or NOT need a "workaround" to be able to run git
> > commands in a repository owned by thyself, is marked as a regression
> > and is expected to be fixed with a future change, which hasn't been
> > provided yet and that is not part of this series.
>
> Do you mean "you can easily unset SUDO_UID to access root-owned
> repositories as root"? Ahh, no, "after tentatively becoming root,
> you can access your own (via SUDO_UID) and root-owned repositories"
> is what you meant, I think. I think that is reasonable to stop the
> current round before adding the support for it.
I thought so too, but now I am not sure anymore; it would seem this is
a "regression" worth fixing by some (especially since there is little
appetite for behaviour changes since the last CVE that had a "fixup"
on top) and the code is available to do so, so will add it as an
"optional" patch on top and then we can decide.
> > --- /dev/null
> > +++ b/t/lib-sudo.sh
> > @@ -0,0 +1,12 @@
> > +# Helpers for running git commands under sudo.
> > +
> > +# Runs a scriplet passed through stdin under sudo.
> > +run_with_sudo () {
> > + local ret
> > + local RUN="$TEST_DIRECTORY/$$.sh"
> > + write_script "$RUN" "$TEST_SHELL_PATH"
> > + sudo "$TEST_SHELL_PATH" -c "\"$RUN\""
>
> This is not wrong per-se, but I think
>
> sudo "$RUN"
>
> would be sufficient, wouldn't it?
only because currently we rely in a sudo that defaults to "-s" and
SHELL should be TEST_SHELL_PATH, but that wasn't explicitly tested by
the prerequisite, and so this just makes sure we ALWAYS use the right
shell, even if sudo might not want to normally.
BTW, sudo IS very opinionated, and just like it can ignore PATH when
it thinks that is the most secure option, can also ignore the #! line
in a shell script and use a more secure SHELL for the same reason, or
even not run ANY shell script, so by doing it this convoluted and
ackward way (as explained in the commit message) we ensure it works,
and works the right way, and is indeed how I was planning to "fix" the
'can we use it also when sudo doesn't default to "-s"', which is
obviously not part of this series, but a future one that should also
improve coverage for this test both in CI and for people brave enough
to try to run it locally.
I guess I will remove it in v5 and which seems better again as an RFC
to make sure we can iron out all remaining controversial things, but
thanks again for your thorough review.
Carlo
