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

[<rsbecker@nexbridge.com>]

On April 28, 2022 12:55 PM, Junio C Hamano wrote:
>Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
>
>> +test_description='verify safe.directory checks while running as root'
>> +
>> +. ./test-lib.sh
>> +
>> +if [ "$IKNOWWHATIAMDOING" != "YES" ]; then
>
>Style.
>
>	if test "$IKNOWWHATIAMDOING" != "YES"
>	then
>
>> +is_root() {
>> +	test -n "$1" && CMD="sudo -n"
>> +	test $($CMD id -u) = $(id -u root)
>> +}
>
>Style.
>
>	is_root () {
>		... body ..
>
>But more importantly, why do we need this in the first place?
>SANITY prerequisite checks if the user is running as root or non-root---can't we
>use it here?
>
>Or perhaps my reading is wrong?  I assumed from its name that it was just "see if
>we are running as user 'root' and return 0 or non-zero to answer", but if it does
>more than that, like priming "sudo", then probably it is misnamed.
>
>> +test_lazy_prereq SUDO '
>> +	is_root sudo &&
>> +	! sudo grep -E '^[^#].*secure_path' /etc/sudoers '

/etc/sudoers is not standard although usual. This path should come from a knob somewhere. We can't run this test on our x86 system anyway - no access to root or sudo even though it is installed. Also, /etc/sudoers is typically secured 0600 so the grep will fail even if is_root passes - and I'm worried about the portability of is_root, which is mostly Linux.

>OK.
>
>> +test_lazy_prereq ROOT '
>> +	is_root
>> +'
>> +
>> +test_expect_success SUDO 'setup' '
>> +	sudo rm -rf root &&
>> +	mkdir -p root/r &&
>> +	sudo chown root root &&
>> +	(
>> +		cd root/r &&
>> +		git init
>> +	)
>> +'
>
>We have a root-owned directory "root" with a subdirectory "r" owned by us.  We
>want to be able to use our "root/r" directory as a repository.  OK.
>
>The prerequisite allows this test to be started as root, but I do not quite see the
>point.  It may pass when started as root, but it is not testing what this test is
>designed to check (i.e. an ordinary user who has repository at root/r can do things
>there).
>
>> +test_expect_success SUDO 'sudo git status as original owner' '
>> +	(
>> +		cd root/r &&
>> +		git status &&
>> +		sudo git status
>> +	)
>> +'
>
>And the directory can be used by the user under "sudo", too.  Good.
>
>The same "this is allowed to run as root, but why?" question applies.  If this was
>started by 'root', root, root/r and root/r/.git all are owned by 'root' and we are
>checking if 'root'
>can run 'git status' as 'root' (or 'root' via sudo) there.  Such a test may well pass, but
>it is not catching a future regression on the code you wrote for this series.
>
>> +test_expect_success SUDO 'setup root owned repository' '
>> +	sudo mkdir -p root/p &&
>> +	sudo git init root/p
>> +'
>
>Now we go on to create root owned repository at root/p
>
>> +test_expect_success SUDO,!ROOT 'can access if owned by root' '
>> +	(
>> +		cd root/p &&
>> +		test_must_fail git status
>> +	)
>> +'
>
>And as an ordinary user, we fail to access a repository that is owned by a wrong
>person (i.e. root).  !ROOT (or SANITY) prereq should be there NOT because the
>test written here would fail if run by root, but because running it as root, even if
>passed, is totally pointless, because we are *not* testing "person A has a
>repository, person B cannot access it" combination.
>
>The other side of the same coin is that the lack of !ROOT (or
>SANITY) prereq in earlier tests I pointed out above misses the point of why we
>have prerequisite mechanism in the first place.  It is not to mark a test that fails
>when the precondition is not met.  It is to avoid running code that would NOT test
>what we want to test.
>
>The difference is that a test that passes for a wrong reason (e.g. we wanted to see
>of person A can access a repository of their own even when the user identity is
>tentatively switched to 'root'
>via 'sudo'---if person A is 'root', the access will be granted even if the special code
>to handle 'sudo' situation we have is broken) should also be excluded with
>prerequisite.
>
>> +test_expect_success SUDO,!ROOT 'can access with sudo' '
>> +	# fail to access using sudo
>> +	(
>> +		# TODO: test_must_fail missing functionality
>
>Care to explain a bit in the log message or in this comment the reason why we do
>not use test_must_fail but use ! here?  Are we over-eager to reject anything non
>"git" be fed, or something?
>
>> +		cd root/p &&
>> +		! sudo git status
>> +	)
>> +'
>
>The repository is owned by 'root', but because of the 'sudo'
>"support", you cannot access the repository with "sudo git".
>
>The test title needs updating.  We expect that the repository cannot be accessed
>under sudo.
>
>> +test_expect_success SUDO 'can access with workaround' '
>
>"workarounds", I think.
>
>> +	# provide explicit GIT_DIR
>> +	(
>> +		cd root/p &&
>> +		sudo sh -c "
>> +			GIT_DIR=.git GIT_WORK_TREE=. git status
>> +		"
>> +	) &&
>> +	# discard SUDO_UID
>> +	(
>> +		cd root/p &&
>> +		sudo sh -c "
>> +			unset SUDO_UID &&
>> +			git status
>> +		"
>> +	)
>> +'
>
>Again, this lack !ROOT (or SANITY) because tests pass for a wrong reason.
>
>Overall, I like the simplicity and clarity of "do not start this test as 'root'" in the
>previous round much better.
>
>But other than that, the test coverage given by this patch looks quite sensible.
>
>Thanks.
>
>> +
>> +test_expect_success SUDO 'cleanup' '
>> +	sudo rm -rf root
>> +'
>> +
>> +test_done