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

[Junio C Hamano <gitster@pobox.com>]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> 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.

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

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

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

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

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

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

> +	(
> +		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
	)

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

OK.

Thanks.