Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 29, 2022, 18:50 UTC
Message-ID: <xmqqee1fbu67.fsf@gitster.g>
In-Reply-To: <20220429012438.37o4uaxsrfdu2b6x@carlos-mbp.lan>

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> If we do :
>
>   [0] login as regular user || sudo to root || login as root

Among these three, the last one is equivalent to "sudo and then unset the environment", right?

As many installations no longer allow direct "login" as "root", clarifying how users can get the behaviour for the third column would help our documentation, and that is the reason behind the question. IOW, this is merely a simple yes-or-no question to sanity-check our mutual understanding, and no need to get overly defensive about being asked.

Show 13 quoted lines

>   [1] % mkdir -p root/r
>   [2] % git init root/r
>   [3] % cd root/r && git status
>
>   step \ type | regular user | sudo to root | root
> --------------------------------------------------
>             1 |    work      |     work     | work
>   before    2 |    work      |     work     | work
>             3 |    work      |     work     | work
> ---------------------------------------------------
>             1 |    work      |     work     | work
>    after    2 |    work      |     work     | work
>             3 |    work      |     fail     | work

So the only difference is that in a repository created by a user who did "sudo mkdir; sudo git init". It used to be that the same user can read the repository with "sudo git status" (because we did not care about how we become 'root', we only saw the owner of the repo and the current euid). Now, such an access is no longer allowed.

And a workaround is to use the third-column behaviour, i.e. either login as root (which is probably too cumbersome as a step in a typical "make && make test && make install" sequence where at least the last step need to be done as a privileged user) or use "sudo" and drop the SUDO_UID environment, for which, the documentation was added in this series.

But I do not see what relevance the above has to the argument you were making, against "if you start these tests as 'root' (instead of starting as an ordinary user), some tests may succeed but for a wrong reason, and some tests may fail because they are not prepared for it; it is wrong to mark only the latter with prerequisite and not the former". The change in the behaviour we see above is for those who start as an ordinary user and uses "sudo" without dropping SUDO_UID. How is it relevant to allow those who start the test as 'root' (not an ordinary user) to try that? Tests done under such condition will see 'root' in euid, SUDO_UID, and st.st_uid, so there is no way for them to detect any mismatch and behave differently, so the transition from "it used to work" to "now it is forbidden" is not even tested.

Show 8 quoted lines

> and rejects the repository because it is NOT owned by that id (it was created
> by root anyway, even if there is no way for git to know that it was done
> at a different time and with a different session, and therefore the SUDO_UID
> variable it is honoring could be considered irrelevant in the current context.
>
> in the documentation patch (which I think would be better to squash with the
> fix) I explain what to do as a workaround, and I expect this use case to be
> less common than the currently broken one (so a net positive)

Both of these two paragraphs speak truth, but there is no relevance to, and it does not justify, your "I disagree, and think that the fact ... proves my point".

For example, this is the 'setup' step.

Show 9 quoted lines

> +test_expect_success SUDO 'setup' '
> +	sudo rm -rf root &&
> +	mkdir -p root/r &&
> +	sudo chown root root &&
> +	(
> +		cd root/r &&
> +		git init
> +	)
> +'

If the test was started by an ordinary user, root/r is owned by the user who is not 'root'. If the test was started by 'root', everything is owned by 'root'. Either way, 'root' is owned by 'root'. In such a repository, we see this test:

+test_expect_success SUDO 'sudo git status as original owner' ' + ( + cd root/r && + git status && + sudo git status + ) +'

The behaviour we are trying to ensure is that, even though root/r is owned by non-root, accessing it with "git status" as the original user and "git status" as root work, as long as if you used "sudo" in the second "git status", so that "git status" can take SUDO_UID into account. The test is making sure that our "pay attention to SUDO_UID" mechanism has not been broken by future changes.

If we start this test as 'root', we cannot test for that. The setup step made everything owned by 'root', and we go there as 'root' and run "git status", which should succceed, but with "sudo git status", even we broke SUDO_UID mechanism and compared euid with st.st_uid, we'll allow an access.

So the test may succeed but it succeeds for a wrong reason even after we break the mechanism added by this series.