Re: [PATCH v3 1/3] t: document regression git safe.directory when using sudo

[Carlo Arenas <carenas@gmail.com>]

On Thu, May 5, 2022 at 6:44 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Mon, 2 May 2022, Carlo Marcelo Arenas Belón wrote:
> >
> >   $ IKNOWWHATIAMDOING=YES ./t0034-root-safe-directory.sh
>
> Hmm. I would like to suggest that we can side-step all of these issues
> (and the ones I outline below) by considering a similar approach to the
> one Stolee took in t0033: use one or more `GIT_TEST_*` environment
> variables to pretend the exact scenario we want to test for.

I wish we could, but I think it is not possible in principle because
it would break the trust chain that we are relying on here to make
this work.

As explained in the commit message from the next patch, for this to be
useful as well as safe our ONLY chance is to trust SUDO_UID hasn't
been tampered with, which also requires that we run through sudo so
git is running as root and not as the regular user the test suite
would use.

If we remove the (I am really root, before I trust SUDO_UID)
requirement from our code, we have just opened ourselves to a way to
weaken the protections that were added with CVE-2022-24765.

to be frank while Junio mention this "weakens" the checks, I consider
I was strengthened them by introducing a mechanism the user could use
(only when he is root) to safely tell us that he wants to trust a
repository that is not owned by him without having to create an
exception, and also improving the usability of it, but "magically"
detecting which uid they are most likely to trust.

> It took me a good chunk of time to figure out how to run these tests, and
> I will have to remember to revert the temporary edit of `/etc/sudoers`
> file. This is definitely not something I plan on doing often, so I wonder
> how these regression tests can guarantee that no regressions are
> introduced if they are so hard to run ;-)

by running in the CI (at least the macOS hosts, and maybe others if we
decide later to butcher their sudoers config as well)
I am adding more instructions in the commit message from the next RFC
to help anyone that might want to run this locally (which I
wouldn't recommend myself)

> This would be more canonical as `test_when_finished "sudo rm -rf root"` in
> the preceding test cases.

correct, but I was attempting not to do that to make it less of a pain
to write more tests since (probably incorrectly) I assumed it would be
simpler to remember that there is a test at the end that does the
cleanup and at least one at the beginning that does the setup than
probably having to take care of those 2 things on each test that you
write.

Ideally, the test framework would be able to know that this test
creates files as root and cleanup itself, but that was specifically
punted.

I am keeping this for the next RFC, but I am open to changing it to
whatever you would prefer, until a proper integration could be written
to clean that mess up.

Carlo