| Message ID | 20220428105852.94449-4-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix `sudo make install` regression in maint | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > +test_description='verify safe.directory checks while running as root' > + > +. ./test-lib.sh > + > +if [ "$IKNOWWHATIAMDOING" != "YES" ]; then Style. if test "$IKNOWWHATIAMDOING" != "YES" then > +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. > +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
On 28/04/2022 17:55, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> 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
Phillip Wood <phillip.wood123@gmail.com> writes: > On 28/04/2022 17:55, Junio C Hamano wrote: >> Carlo Marcelo Arenas Belón <carenas@gmail.com> 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. If this weren't "let's reuse the same mechanism as already used in 1509", I would have had the same reaction. Renaming would be better done outside the topic, I would think. Thanks.
On April 28, 2022 12:55 PM, Junio C Hamano wrote: >Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > >> +test_description='verify safe.directory checks while running as root' >> + >> +. ./test-lib.sh >> + >> +if [ "$IKNOWWHATIAMDOING" != "YES" ]; then > >Style. > > if test "$IKNOWWHATIAMDOING" != "YES" > then > >> +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. > >> +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. >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
On Thu, Apr 28, 2022 at 12:53 PM <rsbecker@nexbridge.com> wrote: > /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. correct and note that the test would succeed if the file doesn't exist because what we are really interested on, is to make sure that sudo won't mess with our path when invoking git, and if there is a chance it would (because that setting is enabled in a different file for example) then we will just skip these tests. Obviously the target I had in mind when I built this test was my own workstation and our public CI, but feel free to provide a fixup that would also make it work for your private runs if you are interested in also running this test. > Also, /etc/sudoers is typically secured 0600 so the grep will fail even if is_root passes It won't, because it is running with sudo ;). note that as stated in the commit message, this requires a fairly open sudo setup (like the one github provides with their actions). > - and I'm worried about the portability of is_root, which is mostly Linux. I actually made sure that is_root was posix shell compatible, but got a little carried away when refactoring it to accomodate for reuse; eitherway it is gone in v3. Carlo
<rsbecker@nexbridge.com> 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.
On April 28, 2022 4:32 PM, Junio C Hamano wrote: >To: rsbecker@nexbridge.com >Cc: 'Carlo Marcelo Arenas Belón' <carenas@gmail.com>; git@vger.kernel.org >Subject: Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo > ><rsbecker@nexbridge.com> 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.
On April 28, 2022 4:23 PM, Carlo Arenas wrote: >On Thu, Apr 28, 2022 at 12:53 PM <rsbecker@nexbridge.com> wrote: >> /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. > >correct and note that the test would succeed if the file doesn't exist because what >we are really interested on, is to make sure that sudo won't mess with our path >when invoking git, and if there is a chance it would (because that setting is enabled >in a different file for >example) then we will just skip these tests. > >Obviously the target I had in mind when I built this test was my own workstation >and our public CI, but feel free to provide a fixup that would also make it work for >your private runs if you are interested in also running this test. > >> Also, /etc/sudoers is typically secured 0600 so the grep will fail >> even if is_root passes > >It won't, because it is running with sudo ;). note that as stated in the commit >message, this requires a fairly open sudo setup (like the one github provides with >their actions). > >> - and I'm worried about the portability of is_root, which is mostly Linux. > >I actually made sure that is_root was posix shell compatible, but got a little carried >away when refactoring it to accomodate for reuse; eitherway it is gone in v3. I tried to find is_root in POSIX but could not. Do you have a reference? It is not in bash 4.3.48, which is on our older system.
Carlo Arenas <carenas@gmail.com> writes: > It won't, because it is running with sudo ;). note that as stated in > the commit message, this requires a fairly open sudo setup (like the > one github provides with their actions). Ahh, OK. So this is pretty much only for CI environment and such, not on a typical developer and end-user box. OK, but the potential issues that I raised about cleansed environment, not limited to $PATH, still exists. Perhaps the prereq check can be tightened to something like: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=no-no-name \ PATH=no-such-path:$PATH \ sudo sh -c "echo '\$PATH \$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME'" | grep "^no-such-path.* no-no-name$" i.e. we export an envionment with a known value that is unlikely value for the variable in tester's environment, prepend a known string that unlikely begins the tester's $PATH, and ask sudo what values, if any, the process sudo spawned sees in these two environment variables. An environment that does not molest PATH and passes environment variables we set in the tests to affect execution of "git" being tested will pass the above test. Otherwise the environment would silently be breaking our expectation.
On Thu, Apr 28, 2022 at 1:32 PM Junio C Hamano <gitster@pobox.com> wrote: > 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. our CI runs with macOS should still work, and any place that does is better that none IMHO > 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. sadly the issue is even more insidious because the PATH is not changed, but sudo ignores it when going to look for a binary to run > $ 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. I think we are confusing, running the whole suite with sudo vs running a specific command inside a test with it. We obviously will need to adapt our suite a lot more before it will natively support sudo and that might never happen as well, since there are conflicting objectives there. IMHO that doesn't mean we shouldn't try. Maybe I should document better what I meant with "awkward", but yes I was serious when I said "minimal support" for running things through sudo, but I should mention the ones you provided almost worked AS-IS so it is not that bad IMHO. Carlo
<rsbecker@nexbridge.com> writes: >>I actually made sure that is_root was posix shell compatible, but got a little carried >>away when refactoring it to accomodate for reuse; eitherway it is gone in v3. > > I tried to find is_root in POSIX but could not. Do you have a reference? It is not in bash 4.3.48, which is on our older system. What he meant was the implementation of is_root shell function he wrote in the patch uses construct from POSIX. is_root() { test -n "$1" && CMD="sudo -n" test $($CMD id -u) = $(id -u root) } Besides, as somebody else already pointed out, this will run random command that is in $CMD (perhaps from tester's environment) when it is run without $1 or an empty string in $1. But other than that, "id" being in POSIX.1, that looks fairly safe. Of course, sudo and sudo -n would not be in POSIX, but that is what this one is testing availablity for, so it is to be expected ;-)
On Thu, Apr 28, 2022 at 1:43 PM <rsbecker@nexbridge.com> wrote:
> I tried to find is_root in POSIX but could not. Do you have a reference? It is not in bash 4.3.48, which is on our older system.
my bad; is_root is a helper function i provided as part of this file;
the latest version which should work in your posix system AND was
specifically written to hopefully not break with NON-STOP based on
what you told us about it looks like (hand edited and not tested) :
is_root() {
id -u >u
id -u root >r
cmp u r
}
On Thu, Apr 28, 2022 at 9:55 AM Junio C Hamano <gitster@pobox.com> wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > +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? my bad; tried first to use NON_ROOT but it didn't work and SANITY seems way too complicated for what is really needed, plus this can be shared by both prerequisites, and more importantly allows me to introduce an exploit with that CMD trick, but Phillip's eagle eyes already blocked me so it is gone and replaced with SANITY for v3 > 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. it does both indeed, and is also why I am pulling the SUDO prerequisite on each test instead of checking once at the beginning of the file and being done with it. I would rather have some tests skipped if sudo can't get root without password than a failed test, and want sudo to always work and not "possibly hang, waiting for a password" during each run, not to block CI either. > 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. I have to agree that I was looking at it the other way, my concern with allowing root to call this was to make sure that none of my changes (or any future ones) prevent them to do what they should normally do, hence why I only disabled for root the tests that couldn't work because make no sense (just like is done for tests that rely on a case insensitive filesystem, for example) More on that in the next test. > 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 a repository at root/r can do things there). IMHO these tests should validate that ANY user can do what is expected, and that includes root (the two versions of it), not only "regular users" > > +test_expect_success SUDO 'sudo git status as original owner' ' > > + ( > > + cd root/r && > > + git status && > > + sudo git status > > + ) > > +' > 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. It is subtle but it does, when run as a real root it will pass, but if we run it through sudo it fails because of the change that was introduced in this series. > > +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. Agree I am abusing the prerequisites, I am instead removing the tests since they are pointless when run as root in v3, which would have been part of the first proposal, even if slightly more complicated. > > +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? correct since 6a67c759489 (test-lib-functions: restrict test_must_fail usage, 2020-07-07) using anything but git fails, and improving that now is IMHO not worth it. The only protection we get from using test_must_fail instead would be to know if we introduced a crash, but the same command has run several times already so IMHO it is unlikely. will obviously not miss the opportunity to enhance test_must_fail and remove the TODO otherwise ASAP. > Overall, I like the simplicity and clarity of "do not start this > test as 'root'" in the previous round much better. I disagree, and think that the fact that the second test changes behaviour with this series proves my point. I agree I was abusing the prerequisites, but was in the name of making this change simpler, I am hoping and slightly more complicated version that doesn't abuse them would be better than having a simpler one where those issues are hidden and even if we currently have no "run as root CI jobs" and last time I tried one found it is broken somewhere else. Either Way those issues are orthogonal to this change and would be happy to discuss again after v3 which is still not ready and will be posted most likely as an RFC including as much as can from the feedback provided so far. Carlo
Carlo Arenas <carenas@gmail.com> writes: >> Overall, I like the simplicity and clarity of "do not start this >> test as 'root'" in the previous round much better. > > I disagree, and think that the fact that the second test changes > behaviour with this series proves my point. I do not know which second test you are talking about, but anyway. > Either Way those issues are orthogonal to this change and would be > happy to discuss again after v3 which is still not ready and will be > posted most likely as an RFC including as much as can from the > feedback provided so far. Sure. Looking forward to seeing a version that does *NOT* abuse prerequisite mechanism. We are interested in testing an ordinary user becoming root via sudo in these tests, so reject whoever starts the test under 'root' upfront like in the earlier round. Thanks.
On April 28, 2022 4:56 PM, Carlo Arenas wrote: >On Thu, Apr 28, 2022 at 1:43 PM <rsbecker@nexbridge.com> wrote: >> I tried to find is_root in POSIX but could not. Do you have a reference? It is not in >bash 4.3.48, which is on our older system. > >my bad; is_root is a helper function i provided as part of this file; the latest version >which should work in your posix system AND was specifically written to hopefully >not break with NON-STOP based on what you told us about it looks like (hand >edited and not tested) : > >is_root() { > id -u >u > id -u root >r > cmp u r >} This is about as portable as I can find and works even in ksh. It could be optimized. is_root() { id -u >u id -u root >r cmp -s u r if [ $? -ne 0 ]; then echo 0 else echo 1 fi } if [ `is_root` -ne 0 ]; then echo root else echo Not root fi
<rsbecker@nexbridge.com> writes: >>is_root() { >> id -u >u >> id -u root >r >> cmp u r >>} > > This is about as portable as I can find and works even in ksh. It could be optimized. > > is_root() { > id -u >u > id -u root >r > cmp -s u r > if [ $? -ne 0 ]; then > echo 0 > else > echo 1 > fi > } > > if [ `is_root` -ne 0 ]; then > echo root > else > echo Not root > fi The above looks very roundabout way. With the first three in is_root that ends with "cmp", we already know from its exit status if "id -u" output for ourselves matches that for root, i.e. if we are root then cmp would have exited with 0. So with the first one I quoted from your quote, the caller can say if is_root then echo root else echo not root fi without turning the exit status into string "0" or "1" and comparing that string with "[ `cmd` -ne 0 ]". And the first one is just as portable. I agree that running cmp with "-s" is probably a good idea. What I used to often use in my previous life (in previous century) is technically incorrect, but is a lot more succinct and works well in practice on any sanely installed systems. Just see if the root directory is writable. No sane system makes it writable by anybody but root. I.e. if test -w / then ... we are running as root ... else ... we are not running as root ... fi
On April 28, 2022 6:22 PM, Junio C Hamano wrote: ><rsbecker@nexbridge.com> writes: > >>>is_root() { >>> id -u >u >>> id -u root >r >>> cmp u r >>>} >> >> This is about as portable as I can find and works even in ksh. It could be >optimized. >> >> is_root() { >> id -u >u >> id -u root >r >> cmp -s u r >> if [ $? -ne 0 ]; then >> echo 0 >> else >> echo 1 >> fi >> } >> >> if [ `is_root` -ne 0 ]; then >> echo root >> else >> echo Not root >> fi > >The above looks very roundabout way. With the first three in is_root that ends >with "cmp", we already know from its exit status if "id -u" output for ourselves >matches that for root, i.e. if we are root then cmp would have exited with 0. > >So with the first one I quoted from your quote, the caller can say > > if is_root > then > echo root > else > echo not root > fi > >without turning the exit status into string "0" or "1" and comparing that string with >"[ `cmd` -ne 0 ]". And the first one is just as portable. I agree that running cmp >with "-s" is probably a good idea. > >What I used to often use in my previous life (in previous century) is technically >incorrect, but is a lot more succinct and works well in practice on any sanely >installed systems. Just see if the root directory is writable. No sane system makes >it writable by anybody but root. > >I.e. > > if test -w / > then > ... we are running as root ... > else > ... we are not running as root ... > fi I agree. I think my ksh is rather broken. But we use bash for git testing and can never go back to ksh, so no worries on that score. Thanks.
On Thu, Apr 28, 2022 at 02:07:56PM -0700, Junio C Hamano wrote: > Carlo Arenas <carenas@gmail.com> writes: > > >> Overall, I like the simplicity and clarity of "do not start this > >> test as 'root'" in the previous round much better. > > > > I disagree, and think that the fact that the second test changes > > behavior with this series proves my point. > > I do not know which second test you are talking about, but anyway. My bad; by "second test" I was referring to the one that I introduced to track the regression and its fix and which has its behavior changed, but you would only notice if looking at it from all angles (and yes, that includes as a regular user, as well as root, with and without sudo : If we do : [0] login as regular user || sudo to root || login as root [1] % mkdir -p root/r [2] % git init root/r [3] % cd root/r && git status we get step \ type | regular user | sudo to root | root -------------------------------------------------- 1 | work | work | work before 2 | work | work | work 3 | work | work | work --------------------------------------------------- 1 | work | work | work after 2 | work | work | work 3 | work | fail | work the reason why it fails is expected (git now finds the SUDO_UID variable and rejects the repository because it is NOT owned by that id (it was created by root anyway, even if there is no way for git to know that it was done at a different time and with a different session, and therefore the SUDO_UID variable it is honoring could be considered irrelevant in the current context. in the documentation patch (which I think would be better to squash with the fix) I explain what to do as a workaround, and I expect this use case to be less common than the currently broken one (so a net positive) Carlo
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > If we do : > > [0] login as regular user || sudo to root || login as root Among these three, the last one is equivalent to "sudo and then unset the environment", right? As many installations no longer allow direct "login" as "root", clarifying how users can get the behaviour for the third column would help our documentation, and that is the reason behind the question. IOW, this is merely a simple yes-or-no question to sanity-check our mutual understanding, and no need to get overly defensive about being asked. > [1] % mkdir -p root/r > [2] % git init root/r > [3] % cd root/r && git status > > step \ type | regular user | sudo to root | root > -------------------------------------------------- > 1 | work | work | work > before 2 | work | work | work > 3 | work | work | work > --------------------------------------------------- > 1 | work | work | work > after 2 | work | work | work > 3 | work | fail | work So the only difference is that in a repository created by a user who did "sudo mkdir; sudo git init". It used to be that the same user can read the repository with "sudo git status" (because we did not care about how we become 'root', we only saw the owner of the repo and the current euid). Now, such an access is no longer allowed. And a workaround is to use the third-column behaviour, i.e. either login as root (which is probably too cumbersome as a step in a typical "make && make test && make install" sequence where at least the last step need to be done as a privileged user) or use "sudo" and drop the SUDO_UID environment, for which, the documentation was added in this series. But I do not see what relevance the above has to the argument you were making, against "if you start these tests as 'root' (instead of starting as an ordinary user), some tests may succeed but for a wrong reason, and some tests may fail because they are not prepared for it; it is wrong to mark only the latter with prerequisite and not the former". The change in the behaviour we see above is for those who start as an ordinary user and uses "sudo" without dropping SUDO_UID. How is it relevant to allow those who start the test as 'root' (not an ordinary user) to try that? Tests done under such condition will see 'root' in euid, SUDO_UID, and st.st_uid, so there is no way for them to detect any mismatch and behave differently, so the transition from "it used to work" to "now it is forbidden" is not even tested. > and rejects the repository because it is NOT owned by that id (it was created > by root anyway, even if there is no way for git to know that it was done > at a different time and with a different session, and therefore the SUDO_UID > variable it is honoring could be considered irrelevant in the current context. > > in the documentation patch (which I think would be better to squash with the > fix) I explain what to do as a workaround, and I expect this use case to be > less common than the currently broken one (so a net positive) Both of these two paragraphs speak truth, but there is no relevance to, and it does not justify, your "I disagree, and think that the fact ... proves my point". For example, this is the 'setup' step. > +test_expect_success SUDO 'setup' ' > + sudo rm -rf root && > + mkdir -p root/r && > + sudo chown root root && > + ( > + cd root/r && > + git init > + ) > +' If the test was started by an ordinary user, root/r is owned by the user who is not 'root'. If the test was started by 'root', everything is owned by 'root'. Either way, 'root' is owned by 'root'. In such a repository, we see this test: +test_expect_success SUDO 'sudo git status as original owner' ' + ( + cd root/r && + git status && + sudo git status + ) +' The behaviour we are trying to ensure is that, even though root/r is owned by non-root, accessing it with "git status" as the original user and "git status" as root work, as long as if you used "sudo" in the second "git status", so that "git status" can take SUDO_UID into account. The test is making sure that our "pay attention to SUDO_UID" mechanism has not been broken by future changes. If we start this test as 'root', we cannot test for that. The setup step made everything owned by 'root', and we go there as 'root' and run "git status", which should succceed, but with "sudo git status", even we broke SUDO_UID mechanism and compared euid with st.st_uid, we'll allow an access. So the test may succeed but it succeeds for a wrong reason even after we break the mechanism added by this series.
On Fri, Apr 29, 2022 at 11:50:40AM -0700, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > If we do : > > > > [0] login as regular user || sudo to root || login as root > > Among these three, the last one is equivalent to "sudo and then > unset the environment", right? Correct, the differences from the most likely sequence of commands of a potential tester on their workstation (which as you point, most of the time doesn't allow login in as root) is : $ sudo -s -H # unset SUDO_UID # IKNOWWHATIAMDOING=yes ./t0034-root-safe-directory.sh the unset only relevant for the third option, of course. > As many installations no longer allow direct "login" as "root", > clarifying how users can get the behaviour for the third column > would help our documentation, and that is the reason behind the > question. IOW, this is merely a simple yes-or-no question to > sanity-check our mutual understanding, and no need to get overly > defensive about being asked. Apologies if sounding otherwise, but I can assure I am not offended by any of this questions, and looking forward to improve in the documentation with the help you had provided on reviewing it. I would also like to thank you personally, since I am well aware that as a not native English speaker, and because Spanish is my mother's tongue I usually write things in a way that is not natural or expected and also overly verbose and without expected pauses. So having a second set of eyes on it is specially appreciated. > > [1] % mkdir -p root/r > > [2] % git init root/r > > [3] % cd root/r && git status > > > > step \ type | regular user | sudo to root | root > > -------------------------------------------------- > > 1 | work | work | work > > before 2 | work | work | work > > 3 | work | work | work > > --------------------------------------------------- > > 1 | work | work | work > > after 2 | work | work | work > > 3 | work | fail | work > > So the only difference is that in a repository created by a user who > did "sudo mkdir; sudo git init". It used to be that the same user > can read the repository with "sudo git status" (because we did not > care about how we become 'root', we only saw the owner of the repo > and the current euid). Now, such an access is no longer allowed. Correct, but more importantly we now allow the sequence documented in the test case which is also IMHO more comnon and useful, where the last step might need to run through sudo (unlike the previous ones) > But I do not see what relevance the above has to the argument you > were making, against "if you start these tests as 'root' (instead of > starting as an ordinary user), some tests may succeed but for a > wrong reason, and some tests may fail because they are not prepared > for it; it is wrong to mark only the latter with prerequisite and > not the former". The change in the behaviour we see above is for > those who start as an ordinary user and uses "sudo" without dropping > SUDO_UID. How is it relevant to allow those who start the test as > 'root' (not an ordinary user) to try that? Tests done under such > condition will see 'root' in euid, SUDO_UID, and st.st_uid, so there > is no way for them to detect any mismatch and behave differently, > so the transition from "it used to work" to "now it is forbidden" > is not even tested. Correct again; guess my intention wasn't clear. My concern was that by blocking root, we don't allow for such a test (if it can be written, since as you pointed out is challenging) to be added and we also lose the currently failing run which can be used as an "explanation" of sorts to the question of "is this supported and expected to work?" which I think the tests help answer and regardless of what is in the documentation. > > and rejects the repository because it is NOT owned by that id (it was created > > by root anyway, even if there is no way for git to know that it was done > > at a different time and with a different session, and therefore the SUDO_UID > > variable it is honoring could be considered irrelevant in the current context. > > > > in the documentation patch (which I think would be better to squash with the > > fix) I explain what to do as a workaround, and I expect this use case to be > > less common than the currently broken one (so a net positive) > > Both of these two paragraphs speak truth, but there is no relevance > to, and it does not justify, your "I disagree, and think that the > fact ... proves my point". I ended up doing a bigger refactoring with v3 that split the tests in two and which I think will help also in the long run, but kept the Documentation patch independent since it already has your SO and don't want to waste your time further by having to re-review it. Assuming that there are no more improvements (or even if they are) to be made to the documentation would be OK if I follow my own advice and squash it together with the code change that introduces the change? Apologies for not doing it earlier, but in my defense I would say that I sent the currently applied version originally as an RFC ;) > For example, this is the 'setup' step. > > > +test_expect_success SUDO 'setup' ' > > + sudo rm -rf root && > > + mkdir -p root/r && > > + sudo chown root root && > > + ( > > + cd root/r && > > + git init > > + ) > > +' > > If the test was started by an ordinary user, root/r is owned by the > user who is not 'root'. If the test was started by 'root', > everything is owned by 'root'. Either way, 'root' is owned by > 'root'. In such a repository, we see this test: > > +test_expect_success SUDO 'sudo git status as original owner' ' > + ( > + cd root/r && > + git status && > + sudo git status > + ) > +' > > The behaviour we are trying to ensure is that, even though root/r is > owned by non-root, accessing it with "git status" as the original > user and "git status" as root work, as long as if you used "sudo" in > the second "git status", so that "git status" can take SUDO_UID into > account. The test is making sure that our "pay attention to > SUDO_UID" mechanism has not been broken by future changes. > > If we start this test as 'root', we cannot test for that. The setup > step made everything owned by 'root', and we go there as 'root' and > run "git status", which should succceed, but with "sudo git status", > even we broke SUDO_UID mechanism and compared euid with st.st_uid, > we'll allow an access. Depending of the way you become 'root' the test could fail as well, but IMHO the failing should be expected (and as shown in the table above) was introduced with this change. The gist is that if we started the whole test with sudo, there is no way for git (with the current implementation) to differenciate if that SUDO_UID is relevant only to its current run or to the environment and more importantly what the intentions are from the user that is running on that environment. Alternatively we could ammend the code to allow for such logic but I don't think that would be easier to implement and I don't see much of a benefit. My thinking is that if we are going to let sudo tells us who the user is then we must trust that SUDO_UID indicates also the intention of the user behind that environment and therefore it is correct in this case to fail. If the user really meant to not trust its SUDO_UID and instead use the id we are running as, then he should have removed it from the environment before invoking git as documented. Long term, as you proposed, if we are running as root we might ALSO consider that any root owned repositories should be fine to trust but that change is orthogonal to fixing the 'sudo git <command> in my own repository doesn't work after the last maint release' It should also be important to note that starting the whole test with sudo is not trivial, neither expected, and will also block because of sudo most likely removing the IKNOWWHATIAMDOING environment variable, so whichever way we decide we shouldn't expect someone accidentally failing this test. > So the test may succeed but it succeeds for a wrong reason even > after we break the mechanism added by this series. the test as root without a SUDO_UID would succeed for the wrong reasons so I agree (and again apologize for suggesting it) that using a prerequisite to squash that run (as done in the other tests) was wrong. what I am still not sure about is if it is worth complicating the test by adding logic that differenciate a root user with and without SUDO_UID that might run it, and I have to admit that the current draft I have of v3 does follow your advice of blocking root from even being able to run it as suggested (and which is also what I am leaning for). Carlo PS. I apologize for not trimming on your responses more aggresively to save some bytes as I would normally do, but wanted to make sure you understand that by trimming those parts I wasn't implying they weren't read or relevant, but just not strictly neccesary and implictly agreed upon for further discussion
On Thu, Apr 28, 2022 at 11:12 AM Junio C Hamano <gitster@pobox.com> wrote: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > On 28/04/2022 17:55, Junio C Hamano wrote: > >> Carlo Marcelo Arenas Belón <carenas@gmail.com> 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. > > If this weren't "let's reuse the same mechanism as already used in > 1509", I would have had the same reaction. Renaming would be better > done outside the topic, I would think. Since I am renaming it anyway as part of this topic with RFC v4, would it be a good idea to require both? I see the "IKNOWHATIAMDOING" not as a normal GIT_TEST flag, but as a "here be dragons!" warning, and I later found that I either misremembered it being enabled in the CI, or it was dropped with one of those refactors we do often there. My RFC v4 includes a new nice looking GIT_TEST variable as suggested by Phillip which I am also enabling in the CI to hopefully make it even more clear that this is only meant to run there, but sadly that also means that this patch will likely have a conflict when merged upwards. Alternatively I could not enable the CI in this series that is aimed at maint or at least do it in an independent patch so it could be dropped where it is not strictly needed? Carlo
Carlo Arenas <carenas@gmail.com> writes: > Since I am renaming it anyway as part of this topic with RFC v4, would > it be a good idea to require both? > > I see the "IKNOWHATIAMDOING" not as a normal GIT_TEST flag, but as a > "here be dragons!" warning, and I later found that I either > misremembered it being enabled in the CI, or it was dropped with one > of those refactors we do often there. > > My RFC v4 includes a new nice looking GIT_TEST variable as suggested > by Phillip which I am also enabling in the CI to hopefully make it > even more clear that this is only meant to run there, but sadly that > also means that this patch will likely have a conflict when merged > upwards. This must build from the older mainteance tracks like maint-2.30, so let's keep the changes to absolute minimum, especially since that will become the base for any further usability tweaks (in an earlier round you suggested to cover "doas", and other changes may want to be applied but all of them should be deferred to later changes). I actually think 1/3 and 3/3 are OK. Are there remaining issues in these two patches (which only are tests)? As to 2/3, I think the code is basically already fine, but a simplification like the following on top would be a good idea. * The callers do not care how errno is modified by the call made into extract_id_from_env(); we are potentially clobbering errno by calling getenv(), lstat(), geteuid(), etc, and we have no "preserve errno as the caller had" around them. So let's lose the saved_errno thing. * We clear errno before making strtoul() call, so any non-zero errno must have happeneed in strtoul(), which includes ERANGE. There is no point chekcing the returned value env_id; if it is ULONG_MAX but errno is 0, then the SUDO_UID legitimately is naming a user whose UID is that special value, and it is not an indication of an overflow. With the change squashed in, [2/3] can have Reviewed-by: Junio C Hamano <gitster@pobox.com> Thanks. git-compat-util.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git c/git-compat-util.h w/git-compat-util.h index dfdd3e4f81..43c9cd0b48 100644 --- c/git-compat-util.h +++ w/git-compat-util.h @@ -413,14 +413,11 @@ static inline void extract_id_from_env(const char *env, uid_t *id) if (real_uid && *real_uid) { char *endptr; unsigned long env_id; - int saved_errno = errno; errno = 0; env_id = strtoul(real_uid, &endptr, 10); - if (!errno && !*endptr && env_id <= (uid_t)-1) + if (!*endptr && !errno) *id = env_id; - - errno = saved_errno; } }
On Fri, May 6, 2022 at 2:43 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Arenas <carenas@gmail.com> writes: > > > Since I am renaming it anyway as part of this topic with RFC v4, would > > it be a good idea to require both? > > > > I see the "IKNOWHATIAMDOING" not as a normal GIT_TEST flag, but as a > > "here be dragons!" warning, and I later found that I either > > misremembered it being enabled in the CI, or it was dropped with one > > of those refactors we do often there. > > > > My RFC v4 includes a new nice looking GIT_TEST variable as suggested > > by Phillip which I am also enabling in the CI to hopefully make it > > even more clear that this is only meant to run there, but sadly that > > also means that this patch will likely have a conflict when merged > > upwards. > > This must build from the older mainteance tracks like maint-2.30, so > let's keep the changes to absolute minimum makes sense, but I still unsure about the two questions I had above: * would it make sense to make it run ONLY if both variables are set to YES or is it enough to use one (most likely the GIT_TEST one) that we would enable independently in CI? the advantage I see of having both variables is that it is even less likely to even run accidentally in a desktop somewhere and maybe break that test, and also keeps the meaning I wanted to attach to it with that ugly looking flag that no one should ever try to enable in their workstations unless they really know what they are doing. The advantage of ONLY having the GIT_TEST one is that it will be easier to enable in CI and for whoever wants to play with it on their workstation as well, but might still encourage people trying to make it work and wasting their time. * since we have to enable CI for these to be useful, would that be something to be done in an additional patch as part of this topic branch and you will only pull the commits before it to maint to avoid conflicts, or should it be done completely independently as a mini feature branch that depends on this one that will be pulled to seen and merged downwards from it? somehow offtopic but just curious about your process, presume that if we go with a single topic branch adding it instead as 2/4 would break your flow/scripts since the only way to get that merged would be to cherry-pick, right? > I actually think 1/3 and 3/3 are OK. Are there remaining issues in > these two patches (which only are tests)? The versions of them in RFCv4 have more documentation and are cleaner since they mostly include most of the feedback that was collected on them (even if I am still unsure because it is spread around and difficult to track, hence why I was planning an RFC) I don't think there is anything significantly different though but they are and will need another review (which I am hoping would be uncontroversial) > As to 2/3, I think the code is basically already fine, but a > simplification like the following on top would be a good idea. > > * We clear errno before making strtoul() call, so any non-zero > errno must have happeneed in strtoul(), which includes ERANGE. > There is no point checking the returned value env_id; if it is > ULONG_MAX but errno is 0, then the SUDO_UID legitimately is > naming a user whose UID is that special value, and it is not an > indication of an overflow. true, but the apparent check for ULONG_MAX (which should have a comment added) was really a check not to overflow when assigning the value we got into uid_t, by sacrificing an unlikely to be valid ULONG_MAX as an uid. it also has the intentional side effect of breaking this code if uid_t is signed and hopefully triggering a warning in the process since it would be always false, that way whoever has a system where this type is signed will have to carry their own version of the code and we don't have to deal with the portability of it. lastly (since it really made me proud and would be sad to see it go) it ALSO avoids someone trying to sneak a value that would overflow in one of the most common configurations we will run where sizeof(long) > sizeof(uid_t) && MIN_UID_T >=0, by using an equivalent to MAX_UID_T (which only exists in a few systems and therefore can't be relied on) to discard values that would overflow the range uid_t has. without it, we would probably find ourselves in the future having to deal with an embarrassing bug that others before us had suffered. Carlo
Carlo Arenas <carenas@gmail.com> writes: > makes sense, but I still unsure about the two questions I had above: > > * would it make sense to make it run ONLY if both variables are set to > YES or is it enough to use one (most likely the GIT_TEST one) that we > would enable independently in CI? > > the advantage I see of having both variables is that it is even less > likely to even run accidentally in a desktop somewhere and maybe break > that test, and also keeps the meaning I wanted to attach to it with > that ugly looking flag that no one should ever try to enable in their > workstations unless they really know what they are doing. > > The advantage of ONLY having the GIT_TEST one is that it will be > easier to enable in CI and for whoever wants to play with it on their > workstation as well, but might still encourage people trying to make > it work and wasting their time. Those who want to use it in CI would need to be told (or have to figure it out) by the patch that adds either IKNOWWHATIAMDOING or GIT_TEST_WHATEVER, and as long as the patch does its job well enough, I do not see much difference either way. The only possible difference is if we use IKNOWWHATIAMDOING then a special CI job that may run with tweaked /etc/sudoers would run the test in this series *and* the other test we borrowed IKNOWWHATIAMDOING from, which may not necessarily be what we want to do. > * since we have to enable CI for these to be useful, would that be > something to be done in an additional patch as part of this topic > branch and you will only pull the commits before it to maint to avoid > conflicts, or should it be done completely independently as a mini > feature branch that depends on this one that will be pulled to seen > and merged downwards from it? I'd expect that nobody pays attention to GitHub CI runs on maint-2.30 - maint-2.35 branches when I push out. I am hoping that these fixes are built on maint-2.30 _without_ CI integration (i.e. the SUDO tests won't be run due to lack of IKNOWWHATIAMDOING in the CI environment). A single branch, without CI integration, based on maint-2.30 would be prepared. Let's call that cb/path-owner-check-with-sudo topic. It is merged to another branch, based on v2.36.0. Let's call that cb/test-path-owner-check-with-sudo-in-ci. On that latter branch, changes to CI to tweak /etc/sudoers and set IKNOWWHATIAMDOING would be created. That latter branch will percolate down starting at 'seen', through 'next', 'master' and finally to 'maint'. After all that happens to prove the primary topic (sans CI) is sound, the tip of maint-2.30 would be updated by merging it, i.e. cb/path-owner-check-with-sudo, and then the result would be merged to maint-2.31, ..., percolating upwards to maint-2.35. The resulting maint-2.35 may be merged to 'maint' after that but that should become a "already up-to-date" merge, I would expect, because 'maint' by that time would have got cb/path-owner-check-with-sudo as part of merging cb/test-path-owner-check-with-sudo-in-ci already, and the merge of cb/path-owner-check-with-sudo is the only thing 'maint-2.35' has that hasn't merged to 'maint' at that point. > true, but the apparent check for ULONG_MAX (which should have a > comment added) was really a check not to overflow when assigning the > value we got into uid_t, by sacrificing an unlikely to be valid > ULONG_MAX as an uid. Are you worried about uid_t wider than ulong? strtoul() with !errno test covers the case, doesn't it? SUDO_UID cannot have any integer that cannot be represented in uid_t and if strtoul() does not say ERANGE, we know whatever value in SUID_UID did not overflow ulong. > it ALSO avoids someone trying to sneak a value that would overflow in > one of the most common configurations we will run where sizeof(long) > > sizeof(uid_t) && MIN_UID_T >=0, by using an equivalent to MAX_UID_T Sorry, -ECANNOTPARSE. If strtoul() can parse everything in uid_t then where is the room for overflowing? We are trying to protect an unsuspecting user who temporary has become 'root' via sudo, and not somebody deliberately hurt themselves or others by setting SUDO_UID deliberately to strange values (once you are 'root', you have easier ways to hurt other people).
On Fri, May 06, 2022 at 04:55:36PM -0700, Junio C Hamano wrote: > Carlo Arenas <carenas@gmail.com> writes: > > > true, but the apparent check for ULONG_MAX (which should have a > > comment added) was really a check not to overflow when assigning the > > value we got into uid_t, by sacrificing an unlikely to be valid > > ULONG_MAX as an uid. > > Are you worried about uid_t wider than ulong? strtoul() with !errno > test covers the case, doesn't it? No, I am worried about uid_t narrower than ulong, which is also the most likely scenatio with a typeof(uid_t) == uint32_t > SUDO_UID cannot have any integer > that cannot be represented in uid_t and if strtoul() does not say > ERANGE, we know whatever value in SUID_UID did not overflow ulong. It is a little subtle, but strtoul doesn't warrant always an ERANGE because it tries to be helpful when given a negative integer and returns instead an equivalent unsigned long as per spec[1] (or in this case the commentary from OpenBSD man page which is also easier to link) "If the minus sign was part of the input sequence, the numeric value calculated from the sequence of digits is negated as if by unary minus in the result type, which applies unsigned integer wraparound rules." > > it ALSO avoids someone trying to sneak a value that would overflow in > > one of the most common configurations we will run where sizeof(long) > > > sizeof(uid_t) && MIN_UID_T >=0, by using an equivalent to MAX_UID_T > > Sorry, -ECANNOTPARSE. If strtoul() can parse everything in uid_t > then where is the room for overflowing? So lets assume a 32bit unsigned uid_t, that wraparounds at 2^32+1, if we get a negative value that is equivalent to something bigger than it, or even a positive value bigger than it, then the assignment will overflow unless we keep it in check by that obviously too clever condition that was removed and we MIGHT even assume an uid_t of 0, which is embarrasing[0]. > We are trying to protect an > unsuspecting user who temporary has become 'root' via sudo, and not > somebody deliberately hurt themselves or others by setting SUDO_UID > deliberately to strange values (once you are 'root', you have easier > ways to hurt other people). You are correct for the current code that even has a big warning telling people NEVER to run that function for anyone other than root, but who knows how this will evolve in the future. Removing it also has other sideeffect, like making this code work in incorrect ways if uid_t is signed, which I mentioned before but probably should had been added as a comment, but that was part of the requirements[2] we had when Phillip argued correctly that I was restricting the valid uid to only half was possible in 32bit systems. FWIW, sudo prints the uid using "%u" so using unsigned long makes more sense and all these problems are unlikely to be practical issues now so I am ok taking your code if you insist, but I still think that the original one was safer in case things change in the future or if there is a platform we currently run on with has signed uid_t, so I will keep it in the RFC with hopefully enough comments to convince you. Carlo [0] https://github.com/systemd/systemd/issues/11026 [1] https://man.openbsd.org/strtoul.3 [2] https://lore.kernel.org/git/CAPUEspjoTYtv9K=rvpkFnyGnEz_uxefED820rx09b6qGG93SqA@mail.gmail.com/
diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh new file mode 100755 index 00000000000..fb54a2fb851 --- /dev/null +++ b/t/t0034-root-safe-directory.sh @@ -0,0 +1,87 @@ +#!/bin/sh + +test_description='verify safe.directory checks while running as root' + +. ./test-lib.sh + +if [ "$IKNOWWHATIAMDOING" != "YES" ]; then + skip_all="You must set env var IKNOWWHATIAMDOING=YES in order to run this test" + test_done +fi + +is_root() { + test -n "$1" && CMD="sudo -n" + test $($CMD id -u) = $(id -u root) +} + +test_lazy_prereq SUDO ' + is_root sudo && + ! sudo grep -E '^[^#].*secure_path' /etc/sudoers +' + +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 + ) +' + +test_expect_success SUDO 'sudo git status as original owner' ' + ( + cd root/r && + git status && + sudo git status + ) +' + +test_expect_success SUDO 'setup root owned repository' ' + sudo mkdir -p root/p && + sudo git init root/p +' + +test_expect_success SUDO,!ROOT 'can access if owned by root' ' + ( + cd root/p && + test_must_fail git status + ) +' + +test_expect_success SUDO,!ROOT 'can access with sudo' ' + # fail to access using sudo + ( + # TODO: test_must_fail missing functionality + cd root/p && + ! sudo git status + ) +' + +test_expect_success SUDO 'can access with workaround' ' + # 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 + " + ) +' + +test_expect_success SUDO 'cleanup' ' + sudo rm -rf root +' + +test_done