From: Phillip Wood Date: Thu, 28 Apr 2022 18:08:46 GMT Subject: Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo Message-ID: <5493b2f1-e59d-d91d-ac21-47c93d2996f2@gmail.com> In-Reply-To: On 28/04/2022 17:55, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón writes: > >> +test_description='verify safe.directory checks while running as root' >> + >> +. ./test-lib.sh >> + >> +if [ "$IKNOWWHATIAMDOING" != "YES" ]; then > > Style. > > if test "$IKNOWWHATIAMDOING" != "YES" > then Also naming - we normally prefix test environment variables with GIT_TEST_. IKNOWWHATIAMDOING does not tell us what we are allowing by setting the variable. Something like GIT_TEST_ALLOW_SUDO would tell us what we're letting ourselves in for by setting it. >> +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. I'm confused by this as well. Also if $1 is empty we run whatever happens to be in $CMD. Best Wishes Phillip >> +test_lazy_prereq SUDO ' >> + is_root sudo && >> + ! sudo grep -E '^[^#].*secure_path' /etc/sudoers >> +' > > 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