| Message ID | 20220510174616.18629-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: > Note that the specific test that documents that after the previous > changes, it is no longer possible for root (if obtained through sudo) > to NOT add an exception or NOT need a "workaround" to be able to run git > commands in a repository owned by thyself, is marked as a regression > and is expected to be fixed with a future change, which hasn't been > provided yet and that is not part of this series. Do you mean "you can easily unset SUDO_UID to access root-owned repositories as root"? Ahh, no, "after tentatively becoming root, you can access your own (via SUDO_UID) and root-owned repositories" is what you meant, I think. I think that is reasonable to stop the current round before adding the support for it. > As described in the comments from the test itself, at least one of the > documented workarounds could fail if sudo doesn't allow root to call sudo > or if root is not in sudoers, but that is very unlikely, and more > importantly actually not what is provided by the currently supported sudo > configuration this test already relies on and therefore adding a way to > validate it works in the prerequisite has been punted for now. OK. > diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh > new file mode 100644 > index 0000000000..d8a88fb9db > --- /dev/null > +++ b/t/lib-sudo.sh > @@ -0,0 +1,12 @@ > +# Helpers for running git commands under sudo. > + > +# Runs a scriplet passed through stdin under sudo. > +run_with_sudo () { > + local ret > + local RUN="$TEST_DIRECTORY/$$.sh" > + write_script "$RUN" "$TEST_SHELL_PATH" > + sudo "$TEST_SHELL_PATH" -c "\"$RUN\"" This is not wrong per-se, but I think sudo "$RUN" would be sufficient, wouldn't it? > @@ -19,6 +26,12 @@ test_lazy_prereq SUDO ' > test_cmp u r > ' > > +if ! test_have_prereq SUDO > +then > + skip_all="Your sudo/system configuration is either too strict or unsupported" > + test_done > +fi Quite sensible and understandable. > @@ -37,6 +50,51 @@ test_expect_success SUDO 'sudo git status as original owner' ' > ) > ' > > +test_expect_success SUDO 'setup root owned repository' ' > + sudo mkdir -p root/p && > + sudo git init root/p > +' OK, so root/ is owned by 'root', root/r is owned by the tester, and root/p is owned by 'root'. > +test_expect_success 'cannot access if owned by root' ' > + ( > + cd root/p && > + test_must_fail git status > + ) > +' OK. Mark this point as [A] for future reference. > +test_expect_failure SUDO 'can access with sudo if root' ' > + ( > + cd root/p && > + sudo git status > + ) > +' OK. > +test_expect_success SUDO 'can access with sudo using a workaround' ' > + # run sudo twice; would fail if root is not in sudoers It probably is a good idea to check "you can run nested sudo" before setting SUDO prerequisite. Then we do not have to say "would fail" here. > + ( > + cd root/p && > + sudo sudo git status > + ) && > + # provide explicit GIT_DIR > + ( > + cd root/p && > + run_with_sudo <<-END > + GIT_DIR=.git && > + GIT_WORK_TREE=. && > + export GIT_DIR GIT_WORK_TREE && > + git status > + END > + ) && OK. We probably want to highlight that this "by default you can only access your own repository" is *not* a security measure that protects the repositories---it is a security measure to protect you from potentially malicious repositories by unknowingly stepping into them. To do so, let's go back to point [A] and add a similar "I as a tester can still access repository owned by root, as long as I express that I want to access it explicitly" test after it. i.e. ( cd root/p && GIT_DIR=.git GIT_WORK_TREE=. git status ) > + # discard SUDO_UID > + ( > + cd root/p && > + run_with_sudo <<-END > + unset SUDO_UID && > + git status > + END > + ) > +' OK. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > OK, so > > root/ is owned by 'root', > root/r is owned by the tester, and > root/p is owned by 'root'. One thing I forgot. In these three patches, I didn't see anything that required root/ to be owned by 'root'. So perhaps we can lose the "chown root root" in the earlier test.
On Tue, May 10, 2022 at 4:11 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > Note that the specific test that documents that after the previous > > changes, it is no longer possible for root (if obtained through sudo) > > to NOT add an exception or NOT need a "workaround" to be able to run git > > commands in a repository owned by thyself, is marked as a regression > > and is expected to be fixed with a future change, which hasn't been > > provided yet and that is not part of this series. > > Do you mean "you can easily unset SUDO_UID to access root-owned > repositories as root"? Ahh, no, "after tentatively becoming root, > you can access your own (via SUDO_UID) and root-owned repositories" > is what you meant, I think. I think that is reasonable to stop the > current round before adding the support for it. I thought so too, but now I am not sure anymore; it would seem this is a "regression" worth fixing by some (especially since there is little appetite for behaviour changes since the last CVE that had a "fixup" on top) and the code is available to do so, so will add it as an "optional" patch on top and then we can decide. > > --- /dev/null > > +++ b/t/lib-sudo.sh > > @@ -0,0 +1,12 @@ > > +# Helpers for running git commands under sudo. > > + > > +# Runs a scriplet passed through stdin under sudo. > > +run_with_sudo () { > > + local ret > > + local RUN="$TEST_DIRECTORY/$$.sh" > > + write_script "$RUN" "$TEST_SHELL_PATH" > > + sudo "$TEST_SHELL_PATH" -c "\"$RUN\"" > > This is not wrong per-se, but I think > > sudo "$RUN" > > would be sufficient, wouldn't it? only because currently we rely in a sudo that defaults to "-s" and SHELL should be TEST_SHELL_PATH, but that wasn't explicitly tested by the prerequisite, and so this just makes sure we ALWAYS use the right shell, even if sudo might not want to normally. BTW, sudo IS very opinionated, and just like it can ignore PATH when it thinks that is the most secure option, can also ignore the #! line in a shell script and use a more secure SHELL for the same reason, or even not run ANY shell script, so by doing it this convoluted and ackward way (as explained in the commit message) we ensure it works, and works the right way, and is indeed how I was planning to "fix" the 'can we use it also when sudo doesn't default to "-s"', which is obviously not part of this series, but a future one that should also improve coverage for this test both in CI and for people brave enough to try to run it locally. I guess I will remove it in v5 and which seems better again as an RFC to make sure we can iron out all remaining controversial things, but thanks again for your thorough review. Carlo
Carlo Arenas <carenas@gmail.com> writes: >> > --- /dev/null >> > +++ b/t/lib-sudo.sh >> > @@ -0,0 +1,12 @@ >> > +# Helpers for running git commands under sudo. >> > + >> > +# Runs a scriplet passed through stdin under sudo. >> > +run_with_sudo () { >> > + local ret >> > + local RUN="$TEST_DIRECTORY/$$.sh" >> > + write_script "$RUN" "$TEST_SHELL_PATH" >> > + sudo "$TEST_SHELL_PATH" -c "\"$RUN\"" >> >> This is not wrong per-se, but I think >> >> sudo "$RUN" >> >> would be sufficient, wouldn't it? > > only because currently we rely in a sudo that defaults to "-s" and > SHELL should be TEST_SHELL_PATH, but that wasn't explicitly tested by > the prerequisite, and so this just makes sure we ALWAYS use the right > shell, even if sudo might not want to normally. Ah, OK. So giving "$TEST_SHELL_PATH" to write_script and then running the resulting script explicitly with "$TEST_SHELL_PATH" is belt-and-suspenders kind of defensiveness. This was puzzling without explanation, but starts making sense when explained. Thanks.
diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh new file mode 100644 index 0000000000..d8a88fb9db --- /dev/null +++ b/t/lib-sudo.sh @@ -0,0 +1,12 @@ +# Helpers for running git commands under sudo. + +# Runs a scriplet passed through stdin under sudo. +run_with_sudo () { + local ret + local RUN="$TEST_DIRECTORY/$$.sh" + write_script "$RUN" "$TEST_SHELL_PATH" + sudo "$TEST_SHELL_PATH" -c "\"$RUN\"" + ret=$? + rm -f "$RUN" + return $ret +} diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh index ecd9dca6b3..54f687d787 100755 --- a/t/t0034-root-safe-directory.sh +++ b/t/t0034-root-safe-directory.sh @@ -3,6 +3,7 @@ test_description='verify safe.directory checks while running as root' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-sudo.sh if [ "$GIT_TEST_ALLOW_SUDO" != "YES" ] then @@ -10,6 +11,12 @@ then test_done fi +if ! test_have_prereq NOT_ROOT +then + skip_all="These tests do not support running as root" + test_done +fi + test_lazy_prereq SUDO ' sudo -n id -u >u && id -u root >r && @@ -19,6 +26,12 @@ test_lazy_prereq SUDO ' test_cmp u r ' +if ! test_have_prereq SUDO +then + skip_all="Your sudo/system configuration is either too strict or unsupported" + test_done +fi + test_expect_success SUDO 'setup' ' sudo rm -rf root && mkdir -p root/r && @@ -37,6 +50,51 @@ test_expect_success SUDO 'sudo git status as original owner' ' ) ' +test_expect_success SUDO 'setup root owned repository' ' + sudo mkdir -p root/p && + sudo git init root/p +' + +test_expect_success 'cannot access if owned by root' ' + ( + cd root/p && + test_must_fail git status + ) +' + +test_expect_failure SUDO 'can access with sudo if root' ' + ( + cd root/p && + sudo git status + ) +' + +test_expect_success SUDO 'can access with sudo using a workaround' ' + # run sudo twice; would fail if root is not in sudoers + ( + cd root/p && + sudo sudo git status + ) && + # provide explicit GIT_DIR + ( + cd root/p && + run_with_sudo <<-END + GIT_DIR=.git && + GIT_WORK_TREE=. && + export GIT_DIR GIT_WORK_TREE && + git status + END + ) && + # discard SUDO_UID + ( + cd root/p && + run_with_sudo <<-END + unset SUDO_UID && + git status + END + ) +' + # this MUST be always the last test test_expect_success SUDO 'cleanup' ' sudo rm -rf root
Add a support library that provides one function that can be used to run a "scriplet" of commands through sudo and that helps invoking sudo in the slightly awkward way that is required to ensure it doesn't block the call (if shell was allowed as tested in the prerequisite) and it doesn't run the command through a different shell than the one we intended. Add additional negative tests as suggested by Junio and that use a new workspace that is owned by root. Note that the specific test that documents that after the previous changes, it is no longer possible for root (if obtained through sudo) to NOT add an exception or NOT need a "workaround" to be able to run git commands in a repository owned by thyself, is marked as a regression and is expected to be fixed with a future change, which hasn't been provided yet and that is not part of this series. As described in the comments from the test itself, at least one of the documented workarounds could fail if sudo doesn't allow root to call sudo or if root is not in sudoers, but that is very unlikely, and more importantly actually not what is provided by the currently supported sudo configuration this test already relies on and therefore adding a way to validate it works in the prerequisite has been punted for now. Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- t/lib-sudo.sh | 12 +++++++ t/t0034-root-safe-directory.sh | 58 ++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 t/lib-sudo.sh