Re: [PATCH v4 3/3] t0034: add negative tests and allow git init to mostly work under sudo

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2022, 23:11 UTC
Message-ID: <xmqqczgl56fi.fsf@gitster.g>
In-Reply-To: <20220510174616.18629-4-carenas@gmail.com>

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

Show 6 quoted lines

> 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.

Show 6 quoted lines

> As described in the comments from the test itself, at least one of the
> documented workarounds could fail if sudo doesn't allow root to call sudo
> or if root is not in sudoers, but that is very unlikely, and more
> importantly actually not what is provided by the currently supported sudo
> configuration this test already relies on and therefore adding a way to
> validate it works in the prerequisite has been punted for now.

OK.

Show 14 quoted lines

> diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh
> new file mode 100644
> index 0000000000..d8a88fb9db
> --- /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?

Show 9 quoted lines

> @@ -19,6 +26,12 @@ test_lazy_prereq SUDO '
>  	test_cmp u r
>  '
>  
> +if ! test_have_prereq SUDO
> +then
> +	skip_all="Your sudo/system configuration is either too strict or unsupported"
> +	test_done
> +fi

Quite sensible and understandable.

Show 8 quoted lines

> @@ -37,6 +50,51 @@ test_expect_success SUDO 'sudo git status as original owner' '
>  	)
>  '
>  
> +test_expect_success SUDO 'setup root owned repository' '
> +	sudo mkdir -p root/p &&
> +	sudo git init root/p
> +'

OK, so

	root/ is owned by 'root',
	root/r is owned by the tester, and
	root/p is owned by 'root'.

Show 6 quoted lines

> +test_expect_success 'cannot access if owned by root' '
> +	(
> +		cd root/p &&
> +		test_must_fail git status
> +	)
> +'

OK.

Mark this point as [A] for future reference.

Show 6 quoted lines

> +test_expect_failure SUDO 'can access with sudo if root' '
> +	(
> +		cd root/p &&
> +		sudo git status
> +	)
> +'

OK.

> +test_expect_success SUDO 'can access with sudo using a workaround' '
> +	# run sudo twice; would fail if root is not in sudoers

It probably is a good idea to check "you can run nested sudo" before setting SUDO prerequisite. Then we do not have to say "would fail" here.

Show 14 quoted lines

> +	(
> +		cd root/p &&
> +		sudo sudo git status
> +	) &&
> +	# provide explicit GIT_DIR
> +	(
> +		cd root/p &&
> +		run_with_sudo <<-END
> +			GIT_DIR=.git &&
> +			GIT_WORK_TREE=. &&
> +			export GIT_DIR GIT_WORK_TREE &&
> +			git status
> +		END
> +	) &&

OK. We probably want to highlight that this "by default you can only access your own repository" is *not* a security measure that protects the repositories---it is a security measure to protect you from potentially malicious repositories by unknowingly stepping into them. To do so, let's go back to point [A] and add a similar "I as a tester can still access repository owned by root, as long as I express that I want to access it explicitly" test after it. i.e.

	(
		cd root/p &&
		GIT_DIR=.git GIT_WORK_TREE=. git status
	)

Show 9 quoted lines

> +	# discard SUDO_UID
> +	(
> +		cd root/p &&
> +		run_with_sudo <<-END
> +			unset SUDO_UID &&
> +			git status
> +		END
> +	)
> +'

OK.

Thanks.