On April 28, 2022 4:32 PM, Junio C Hamano wrote: >To: rsbecker@xxxxxxxxxxxxx >Cc: 'Carlo Marcelo Arenas Belón' <carenas@xxxxxxxxx>; git@xxxxxxxxxxxxxxx >Subject: Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo > ><rsbecker@xxxxxxxxxxxxx> writes: > >>>> +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. > >True. > >On a box I happen to be using, the grep gives me > > Defaults secure_path=/usr/sbin:/usr/bin:/sbin:/bin > >and makes the prereq fail. Which is sort-of expected, and I understand why this >check is here, but I suspect that any sane and sensible installations would >reinitialize the path to a fairly restricted one with the mechanism, so we wouldn't >be testing this on majority of places, I am afraid. > >What we really care is that we use the same PATH as the test environment uses, >including "we want to test the binary we just built" (or "at the specified path", >when GIT_TEST_INSTALLED is in effect). I wonder what the right way to carry >$PATH and other environment variables down to whatever "sudo" in the test >runs. > > $ foo=foobla; export foo > $ sudo sh -c set | grep foo; echo $? > 1 > >so resetting PATH from an environment we export, e.g. > > USE_THIS_PATH=$PATH sudo sh -c ' > PATH=$USE_THIS_PATH > ... invoke our git normally here ... > git status > ' > >would not work X-<. Worse yet, other environment variables such as >GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME that we set in our tests may >probably be cleared before "sudo" runs any test command, so rejecting an >installaion whose "sudo" resets PATH with the above check is probably insufficient >to give our tests a reasonable envionment to run in. > >>>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. > >So, I unfortunately have to strike the last sentence from my earlier response. The >tests do mean well, but I doubt they would work in the way the good intentions >planned them to work. sudo on my machines is very specific that it does not preserve much of the environment. This causes all kinds of DLL load issues too. We rarely can build regression test suites that include sudo testing. More than that, it is general policy that password-less sudo is frowned upon, so I'm pretty sure we cannot run this test. When we run the git test suite, it is done by a user who is, by design, excluded from the sudoers file because regression tests happen in a sandbox.
