git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, bagasdotme@gmail.com, phillip.wood123@gmail.com
Subject: Re: [RFC PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo
Date: Mon, 02 May 2022 15:10:09 -0700	[thread overview]
Message-ID: <xmqqo80fvb5q.fsf@gitster.g> (raw)
In-Reply-To: <20220502183920.88982-4-carenas@gmail.com> ("Carlo Marcelo Arenas =?utf-8?Q?Bel=C3=B3n=22's?= message of "Mon, 2 May 2022 11:39:20 -0700")

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

> +# Runs a scriplet passed through stdin under sudo.
> +run_with_sudo () {
> +	local ret
> +	local SH=${1-"$TEST_SHELL_PATH"}
> +	local RUN="$HOME/$$.sh"
> +	{
> +		echo "#!$SH"
> +		echo "set -e"
> +		echo ". \"$HOME/env\""
> +		cat
> +	} >"$RUN" &&
> +	chmod +x "$RUN" &&
> +	sudo "$SH" -c "\"$RUN\""
> +	ret=$?
> +	rm -f "$RUN"
> +	return $ret
> +}

I wonder if write_script can be used for better readability.  It is
especially true as I am going to suggest ripping out $HOME/env stuff
that is not absolutely needed (and its support with this patch looks
inadequate when we do need one).

	local RUN=$HOME/$$.sh &&
	write_script "$RUN" "$TEST_SHELL_PATH" &&
	sudo "$RUN"

or something?

> +# Makes all variables passed as parameters available to the scriplet that
> +# run under sudo with run_with_sudo
> +export_to_sudo () {
> +	while test -n "$1"
> +	do
> +		local v
> +		eval v="\$$1"
> +		echo "$1=$v" >>"$HOME/env"
> +		shift
> +	done
> +}

Two potential issues:

 - This forces the caller to list _all_ the relevant environment
   variables that would ever matter, which would not be feasible and
   would not be maintainable.  For example, by forgetting to export
   GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS, "git" commands run in the
   sudo tests change their behaviour.  Whoever writing a new test
   need to see what obscure GIT_TEST_* thing may affect the test
   they want to write, and whoever enhancing the test framework to
   add new GIT_TEST_* knob need to pay attention to the users of
   export_to_sudo if their new knob need to be exported.

 - I think the assignment to $v under eval is correct, but I am not
   sure the string accumulated in the $HOME/env file is safe to
   eval.  We can pass TEST_DIRECTORY via this mechanism, where the
   value deliberately has a whitespace in it, but if the leading
   path to our source directory had a single-quote in it, it
   probably would not work well.  Of course, any variable that has
   LF in its value would not work without proper quoting.

I think both are not impossible but are hard to do right.  Because I
do not see anything that absolutely needs the $HOME/env mechanism to
work in the rest of the tests in this patch, I am inclined to say
that I'd prefer keeping things simple and only make sure we use the
right $SHELL to run our script (which write_script may help).

> diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh
> index 67dd96b9321..0f79648a2fb 100755
> --- a/t/t0034-root-safe-directory.sh
> +++ b/t/t0034-root-safe-directory.sh
> @@ -3,6 +3,19 @@
>  test_description='verify safe.directory checks while running as root'
>  
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-sudo.sh
> +
> +if [ "$IKNOWWHATIAMDOING" != "YES" ]
> +then
> +	skip_all="You must set env var IKNOWWHATIAMDOING=YES in order to run this test"
> +	test_done
> +fi
> +
> +if ! test_have_prereq NOT_ROOT
> +then
> +	skip_all="No, you don't; these tests can't run as root"
> +	test_done
> +fi

OK.

> @@ -19,6 +32,7 @@ test_lazy_prereq SUDO '
>  test_expect_success SUDO 'setup' '
>  	sudo rm -rf root &&
>  	mkdir -p root/r &&
> +	export_to_sudo GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
>  	sudo chown root root &&
>  	(
>  		cd root/r &&

I know I brought up the "git init" during the previous review, but
as long as the test does not depend on the GIT_TEST_* knob
(i.e. either we say "git init --initial-branch" explicitly, or we do
not rely on the initial branch having a certain name), we do not
have to worry.  We do not check what branch we are on after we do
this test, we do not check what branch "git status" reports that we
are on in later tests, we obviously do not care between main and
master in this test script.

I am tempted to suggest dropping the whole $HOME/env business.

> @@ -34,6 +48,50 @@ test_expect_success SUDO 'sudo git status as original owner' '
>  	)
>  '
>  
> +test_expect_success SUDO 'setup root owned repository' '
> +	sudo mkdir -p root/p &&
> +	run_with_sudo <<-END
> +		git init root/p
> +	END
> +'

OK.

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

OK, but strictly speaking, we do not need the SUDO prerequisite for
this one.  It still need it for the test directories prepared in
previous steps anyway, so perhaps we want one check upfront, just
like we do for NOT_ROOT?

	if ! test_have_prereq SUDO
	then
		skip_all="You do not seem to have a working 'sudo'"
		test_done
	fi

> +test_expect_success SUDO 'cannot access with sudo' '
> +	(
> +		# TODO: test_must_fail needs additional functionality
> +		# 6a67c759489 blocks its use with sudo
> +		cd root/p &&
> +		! sudo git status
> +	)
> +'

OK.  So we cannot by default access root-owned repository by
default, which is OK.  I wonder what happens if we did "sudo sudo
git status".  Perhaps the inner sudo will notice that SUDO_UID is
set in its environment and does not update it to 0?

	... goes and checks ...
	$ sudo sudo sh -c 'echo $SUDO_UID; whoami'
	0
	root

So that gives us another workaround, I guess, which might be even
simpler.

> +test_expect_success SUDO 'can access using a workaround' '
> +	# 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
> +	) &&
> +	# discard SUDO_UID
> +	(
> +		cd root/p &&
> +		run_with_sudo <<-END
> +			unset SUDO_UID
> +			git status
> +		END
> +	)

	# double sudo
	(
		cd root/p &&
		sudo sudo git status
	)

I do not know if it is worth adding this third workaround.

> +'
> +
>  # this MUST be always the last test
>  test_expect_success SUDO 'cleanup' '
>  	sudo rm -rf root

Looking much better otherwise.

  reply	other threads:[~2022-05-02 22:10 UTC|newest]

Thread overview: 170+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-26 18:31 [RFC PATCH] git-compat-util: avoid failing dir ownership checks if running priviledged Carlo Marcelo Arenas Belón
2022-04-26 19:48 ` Derrick Stolee
2022-04-26 19:56   ` Junio C Hamano
2022-04-26 20:10     ` rsbecker
2022-04-26 20:45       ` Carlo Arenas
2022-04-26 21:10         ` Junio C Hamano
2022-04-26 20:12     ` Carlo Arenas
2022-04-26 20:26   ` Carlo Arenas
2022-04-29 16:16   ` Derrick Stolee
2022-04-27  0:05 ` [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-04-27  9:33   ` Phillip Wood
2022-04-27 12:30     ` Phillip Wood
2022-04-27 14:15       ` rsbecker
2022-04-27 15:58       ` Carlo Arenas
2022-04-27 16:14         ` Phillip Wood
2022-04-27 18:54           ` Junio C Hamano
2022-04-27 20:59             ` Carlo Arenas
2022-04-27 21:09               ` rsbecker
2022-04-27 21:25               ` Junio C Hamano
2022-04-28 17:56             ` Phillip Wood
2022-04-27 15:38     ` Carlo Arenas
2022-04-27 15:50       ` rsbecker
2022-04-27 16:19       ` Junio C Hamano
2022-04-27 16:45         ` Carlo Arenas
2022-04-27 17:22         ` Phillip Wood
2022-04-27 17:49           ` rsbecker
2022-04-27 17:54             ` Carlo Arenas
2022-04-27 18:05               ` rsbecker
2022-04-27 18:11                 ` Carlo Arenas
2022-04-27 18:16                   ` rsbecker
2022-04-27 16:31       ` Phillip Wood
2022-04-27 16:54         ` Carlo Arenas
2022-04-27 17:28           ` Phillip Wood
2022-04-27 17:49             ` Carlo Arenas
2022-04-27 22:26   ` [RFC PATCH v2] " Carlo Marcelo Arenas Belón
2022-04-27 22:33     ` Junio C Hamano
2022-04-28  3:35     ` [PATCH 0/2] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-04-28  3:35       ` [PATCH 1/2] Documentation: explain how safe.directory works when running under sudo Carlo Marcelo Arenas Belón
2022-04-28  5:17         ` Junio C Hamano
2022-04-28  5:58           ` Carlo Arenas
2022-04-28  6:41             ` Junio C Hamano
2022-04-28  3:35       ` [PATCH 2/2] t: add tests for safe.directory when running with sudo Carlo Marcelo Arenas Belón
2022-04-28  5:34         ` Junio C Hamano
2022-04-28  4:57       ` [PATCH 0/2] fix `sudo make install` regression in maint Junio C Hamano
2022-04-28 10:58       ` [PATCH v2 0/3] " Carlo Marcelo Arenas Belón
2022-04-28 10:58         ` [PATCH v2 1/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-04-28 18:02           ` Phillip Wood
2022-04-28 18:57             ` Carlo Arenas
2022-04-28 10:58         ` [PATCH v2 2/3] Documentation: explain how safe.directory works when running under sudo Carlo Marcelo Arenas Belón
2022-04-30  6:17           ` Bagas Sanjaya
2022-04-30  6:39             ` Junio C Hamano
2022-04-30 14:15             ` Carlo Marcelo Arenas Belón
2022-04-28 10:58         ` [PATCH v2 3/3] t: add tests for safe.directory when running with sudo Carlo Marcelo Arenas Belón
2022-04-28 16:55           ` Junio C Hamano
2022-04-28 18:08             ` Phillip Wood
2022-04-28 18:12               ` Junio C Hamano
2022-05-06 17:50                 ` Carlo Arenas
2022-05-06 21:43                   ` Junio C Hamano
2022-05-06 22:57                     ` Carlo Arenas
2022-05-06 23:55                       ` Junio C Hamano
2022-05-07 11:57                         ` Carlo Marcelo Arenas Belón
2022-04-28 19:53             ` rsbecker
2022-04-28 20:22               ` Carlo Arenas
2022-04-28 20:43                 ` rsbecker
2022-04-28 20:51                   ` Junio C Hamano
2022-04-28 20:56                   ` Carlo Arenas
2022-04-28 21:55                     ` rsbecker
2022-04-28 22:21                       ` Junio C Hamano
2022-04-28 22:45                         ` rsbecker
2022-04-28 20:46                 ` Junio C Hamano
2022-04-28 20:32               ` Junio C Hamano
2022-04-28 20:40                 ` rsbecker
2022-04-28 20:48                 ` Carlo Arenas
2022-04-28 21:02             ` Carlo Arenas
2022-04-28 21:07               ` Junio C Hamano
2022-04-29  1:24                 ` Carlo Marcelo Arenas Belón
2022-04-29 18:50                   ` Junio C Hamano
2022-04-29 20:05                     ` Carlo Marcelo Arenas Belón
2022-05-02 18:39         ` [RFC PATCH v3 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-02 18:39           ` [RFC PATCH v3 1/3] t: document regression git safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-02 21:35             ` Junio C Hamano
2022-05-02 23:07               ` Carlo Arenas
2022-05-02 18:39           ` [RFC PATCH v3 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-02 18:39           ` [RFC PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo Carlo Marcelo Arenas Belón
2022-05-02 22:10             ` Junio C Hamano [this message]
2022-05-03  0:00               ` Carlo Arenas
2022-05-03  6:54         ` [PATCH v3 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-03  6:54           ` [PATCH v3 1/3] t: document regression git safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-03 14:03             ` Phillip Wood
2022-05-03 15:56               ` Carlo Marcelo Arenas Belón
2022-05-04 11:15                 ` Phillip Wood
2022-05-04 13:02                   ` Carlo Arenas
2022-05-04 14:11                     ` Phillip Wood
2022-05-05 13:44             ` Johannes Schindelin
2022-05-05 14:34               ` Phillip Wood
2022-05-05 15:50               ` Junio C Hamano
2022-05-05 18:33               ` Junio C Hamano
2022-05-05 19:39                 ` Junio C Hamano
2022-05-06 21:03                   ` Carlo Arenas
2022-05-09  8:21                 ` Phillip Wood
2022-05-09 14:51                   ` Carlo Arenas
2022-05-09 15:18                     ` Phillip Wood
2022-05-09 16:01                   ` Junio C Hamano
2022-05-09 16:21                     ` Carlo Arenas
2022-05-06 17:39               ` Carlo Arenas
2022-05-03  6:54           ` [PATCH v3 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-05 14:01             ` Johannes Schindelin
2022-05-05 14:32               ` Phillip Wood
2022-05-06 19:15                 ` Carlo Arenas
2022-05-06 20:00                   ` Junio C Hamano
2022-05-06 20:22                     ` Carlo Arenas
2022-05-06 20:59                       ` Junio C Hamano
2022-05-06 21:40                         ` Carlo Arenas
2022-05-06 21:07                       ` rsbecker
2022-05-05 16:09               ` Junio C Hamano
2022-05-06 20:02               ` Carlo Arenas
2022-05-03  6:54           ` [PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo Carlo Marcelo Arenas Belón
2022-05-03 14:12             ` Phillip Wood
2022-05-03 15:27               ` Junio C Hamano
2022-05-06 16:54               ` Carlo Arenas
2022-05-07 16:35           ` [RFC PATCH v4 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-07 16:35             ` [RFC PATCH v4 1/3] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-07 16:35             ` [RFC PATCH v4 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-07 17:34               ` Junio C Hamano
2022-05-07 18:56                 ` Carlo Marcelo Arenas Belón
2022-05-09 16:54                   ` Junio C Hamano
2022-05-09 17:36                     ` rsbecker
2022-05-09 18:48                     ` Carlo Arenas
2022-05-09 19:16                       ` rsbecker
2022-05-09 19:41                       ` Junio C Hamano
2022-05-07 16:35             ` [RFC PATCH v4 3/3] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-10 14:17             ` [RFC PATCH v4 0/3] fix `sudo make install` regression in maint Phillip Wood
2022-05-10 15:47               ` Carlo Arenas
2022-05-10 17:46             ` [PATCH " Carlo Marcelo Arenas Belón
2022-05-10 17:46               ` [PATCH v4 1/3] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-10 22:10                 ` Junio C Hamano
2022-05-10 23:11                   ` Carlo Arenas
2022-05-10 23:44                     ` Junio C Hamano
2022-05-11  0:56                       ` Carlo Arenas
2022-05-11  1:11                         ` Junio C Hamano
2022-05-10 17:46               ` [PATCH v4 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-10 22:57                 ` Junio C Hamano
2022-05-11  7:34                   ` Carlo Arenas
2022-05-11 14:58                     ` Junio C Hamano
2022-05-10 17:46               ` [PATCH v4 3/3] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-10 23:11                 ` Junio C Hamano
2022-05-10 23:25                   ` Junio C Hamano
2022-05-11 14:04                   ` Carlo Arenas
2022-05-11 15:29                     ` Junio C Hamano
2022-05-13  1:00               ` [PATCH v5 0/4] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-13  1:00                 ` [PATCH v5 1/4] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-06-03 12:12                   ` SZEDER Gábor
2022-05-13  1:00                 ` [PATCH v5 2/4] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-06-03 11:05                   ` SZEDER Gábor
2022-06-03 16:54                     ` Junio C Hamano
2022-06-03 17:34                       ` SZEDER Gábor
2022-05-13  1:00                 ` [PATCH v5 3/4] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-13  1:20                   ` Junio C Hamano
2022-05-14 14:36                     ` Carlo Arenas
2022-05-15 16:54                       ` Junio C Hamano
2022-05-15 19:21                         ` Carlo Arenas
2022-05-16  5:27                           ` Junio C Hamano
2022-05-16 13:07                             ` Carlo Marcelo Arenas Belón
2022-05-16 16:25                               ` Junio C Hamano
2022-05-13  1:00                 ` [PATCH v5 4/4] git-compat-util: allow root to access both SUDO_UID and root owned Carlo Marcelo Arenas Belón
2022-06-15 14:02                   ` Johannes Schindelin
2022-06-17 14:26                     ` Carlo Arenas
2022-06-17 16:00                       ` Junio C Hamano
2022-06-17 20:23                   ` [PATCH v6] " Carlo Marcelo Arenas Belón
2022-06-17 21:02                     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo80fvb5q.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=bagasdotme@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=phillip.wood123@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).