| Message ID | 20220513010020.55361-4-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b9063afda17a2aa6310423c9f7b776c41f753091 |
| Headers | show |
| Series | fix `sudo make install` regression in maint | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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. > > Document a regression that was introduced by previous commits where > root won't be able anymore to access directories they own unless > SUDO_UID is removed from their environment. > > The tests document additional ways that this new restriction could > be worked around and the documentation explains why it might be instead > considered a feature, but a "fix" is planned for a future change. > > 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> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > t/lib-sudo.sh | 15 ++++++++ > t/t0034-root-safe-directory.sh | 62 ++++++++++++++++++++++++++++++++++ > 2 files changed, 77 insertions(+) > create mode 100644 t/lib-sudo.sh Heh. I am a bit surprised that double sudo would become a separate prerequisite, instead of a new part of SUDO prerequisite. After all we expect from SUDO prerequisite quite a lot (e.g. most sane installatios facing end-users will futz with $PATH, but we require not to do so to satisfy the SUDO prereq) and it is already very narrowly targetted to a throw-away CI environment whose sudo basically lets us do anything. But that is not a serious enough "thing" to trigger a reroll. This step looks good to me (others I'll comment later). Thanks.
On Thu, May 12, 2022 at 6:20 PM Junio C Hamano <gitster@pobox.com> wrote: > > Heh. I am a bit surprised that double sudo would become a separate > prerequisite, It is because it goes away in the optional patch 4, since it won't be needed anymore after that. Also, it shared the same test with the other 2 workarounds which moved into their own (one as you suggested is more of a statement of policy that says, git trusts the developer and doesn't check if --git-dir or --worktree (or equivalent means to explicitly identify those parts of your repository) are used. and the other, was always "special" since it was documented as an indicator of what to do which could be considered a least privilege marker as well. without the optional patch that brings it back, root MUST indicate through its use of that (or other "workarounds") that he really meant to access a directory owned by root, and will instead defalt (when appropriate) to use the id of the user that invoked sudo, which has (normally) less privilege. > of a new part of SUDO prerequisite. After all > we expect from SUDO prerequisite quite a lot (e.g. most sane > installatios facing end-users will futz with $PATH, but we require > not to do so to satisfy the SUDO prereq) and it is already very > narrowly targetted to a throw-away CI environment whose sudo > basically lets us do anything. just because I didn't want this to become a bigger change that it was already indeed I'd been "cutting" it since the very beginning, by first dropping DOAS support and then avoiding moving things around so it could be easy to backport. I think I can provide a version of it that might be able to work with less restrictions that it currently has, but that would get us into the "test framework integration" that was specifically punted as well. Carlo
Carlo Arenas <carenas@gmail.com> writes: > On Thu, May 12, 2022 at 6:20 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Heh. I am a bit surprised that double sudo would become a separate >> prerequisite, > > It is because it goes away in the optional patch 4, since it won't be > needed anymore after that. Hmph, it may not be needed, but it should still work, in which case it probably is still worth testing, even with the optional patch #4. No?
On Sun, May 15, 2022 at 9:54 AM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Arenas <carenas@gmail.com> writes: > > > On Thu, May 12, 2022 at 6:20 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Heh. I am a bit surprised that double sudo would become a separate > >> prerequisite, > > > > It is because it goes away in the optional patch 4, since it won't be > > needed anymore after that. > > Hmph, it may not be needed, but it should still work, in which case > it probably is still worth testing, even with the optional patch #4. Just because it works, it doesn't mean we have to test it. IMHO tests are better reserved for things that might not work or that should work, so this gets in the same bucket than "triple sudo" would go, and interestingly enough we could add that test without having to add a new prerequisite too! > No? Your call, my assumption was (since this patch is part of this series, albeit optional), that the "double sudo" need will be short lived and therefore better not to have this test to begin with, or remove it as soon as the need is gone, which in practice would be the time between when this series (without the optional patch) is released, and the time that optional path gets released (maybe as part of another series, since it might be dropped from this one). alternatively we can make it not optional, and then the test will NEVER be needed. Carlo
Carlo Arenas <carenas@gmail.com> writes: >> Hmph, it may not be needed, but it should still work, in which case >> it probably is still worth testing, even with the optional patch #4. > > Just because it works, it doesn't mean we have to test it. Yes. It all depends on the answer to this question: Is it reasonably expected that any half-way intelligent Git user would not be surprised to learn that "sudo sudo git status" would be a way to work on a repository that is owned by root as root? Given that "sudo git status" is a good way to work on a repository that is owned by you as root, perhaps the answer is yes, but I am not a representative sample ;-) If the answer is yes, then we would want to make sure it will continue to work by having a test to protect it from future breakage. If not, and "sudo sudo git" (or worse "sudo sudo sudo git") is something that would be imagined by the most wicked mind and no sane person would imagine it would be a way to achieve something useful, no, it does not have to be protected from any future breakage. So...
On Sun, May 15, 2022 at 10:27:04PM -0700, Junio C Hamano wrote: > Carlo Arenas <carenas@gmail.com> writes: > > >> Hmph, it may not be needed, but it should still work, in which case > >> it probably is still worth testing, even with the optional patch #4. > > > > Just because it works, it doesn't mean we have to test it. > > Yes. It all depends on the answer to this question Not quite, after all this is part of the "git" testsuite and therefore will only apply if it would be testing git's functionality, and in this case it does not. More details below. > Is it > reasonably expected that any half-way intelligent Git user would not > be surprised to learn that "sudo sudo git status" would be a way to > work on a repository that is owned by root as root? Given that > "sudo git status" is a good way to work on a repository that is > owned by you as root, perhaps the answer is yes, but I am not > a representative sample ;-) > > If the answer is yes, then we would want to make sure it will > continue to work by having a test to protect it from future > breakage. If not, and "sudo sudo git" (or worse "sudo sudo sudo > git") is something that would be imagined by the most wicked mind > and no sane person would imagine it would be a way to achieve > something useful, no, it does not have to be protected from any > future breakage. The answer is "yes", but it is because of a misunderstanding (which has nothing to do with intelligence, but just experience with sudo and the type of environment where it runs). * sudo does NOT respect SUDO_UID, indeed is one of those few *NIX tools that doesn't even respect EUID but insist on only trusting the real id. * once you run something through sudo, it creates an environment for you that is based on its security policy and not even the invoking user can change some of the parametersr it uses to do that, only "root" can. * that means that once you invoke the first sudo, then the second runs as root and ignores the SUDO_UID the first sudo creates, so by the time git gets to run, it will only see the SUDO_UID that the one that invoked it creates, and since that sudo was running as root it MUST be the same than a root owned file/directory would use, hence why it works for that root owned repository and would fail in one that is owned by the original user. there is no new functionality or code path difference inside git between the first and second invocation of sudo, the only relevant difference is that the starting environment from the two last processes in that triple chain have different values for the SUDO_UID environment variable. Carlo
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > On Sun, May 15, 2022 at 10:27:04PM -0700, Junio C Hamano wrote: >> Carlo Arenas <carenas@gmail.com> writes: >> >> >> Hmph, it may not be needed, but it should still work, in which case >> >> it probably is still worth testing, even with the optional patch #4. >> > >> > Just because it works, it doesn't mean we have to test it. >> >> Yes. It all depends on the answer to this question > > Not quite, after all this is part of the "git" testsuite and therefore will > only apply if it would be testing git's functionality, and in this case it > does not. It is immaterial if the way how "sudo sudo git" behaves is "git's functionality" or not, because what we care about is what the end user sees as a whole and it does not matter all that much to them where the observed behaviour comes from. The rule is simple. If we care about the behaviour to stay with us over time, we ensure it with a test. If we are certain that no users will depend on such a behaviour and are willing to break them (i.e. users who depend on how "sudo sudo git" behaves, which is an empty set) when we need to update the code, then we don't. And if that changes with and without the optional patch #4, it makes it more important to have test (if we care, that is). Later we may find what patch #4 does is detrimental to user experience and decide to tweak it out (not necessarily with a revert of #4, but doing an equivalent of reverting of it only in the code part and not tests). In any case, as I said in the beginning, this was merely "a bit surprised" and "not a serious enough thing to trigger a reroll", so I will stop wasting our time on this thread. Thanks.
diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh new file mode 100644 index 00000000000..b4d7788f4e5 --- /dev/null +++ b/t/lib-sudo.sh @@ -0,0 +1,15 @@ +# 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" + # avoid calling "$RUN" directly so sudo doesn't get a chance to + # override the shell, add aditional restrictions or even reject + # running the script because its security policy deem it unsafe + 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 6b8ea5357f6..a621f1ea5eb 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 && @@ -36,6 +49,55 @@ 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_success 'can access if addressed explicitly' ' + ( + cd root/p && + GIT_DIR=.git GIT_WORK_TREE=. 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 if root by removing SUDO_UID' ' + ( + cd root/p && + run_with_sudo <<-END + unset SUDO_UID && + git status + END + ) +' + +test_lazy_prereq SUDO_SUDO ' + sudo sudo id -u >u && + id -u root >r && + test_cmp u r +' + +test_expect_success SUDO_SUDO 'can access with sudo abusing SUDO_UID' ' + ( + cd root/p && + sudo sudo git status + ) +' + # this MUST be always the last test test_expect_success SUDO 'cleanup' ' sudo rm -rf root