git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Carlo Arenas <carenas@gmail.com>
To: phillip.wood@dunelm.org.uk
Cc: git@vger.kernel.org, gitster@pobox.com, bagasdotme@gmail.com,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH v3 1/3] t: document regression git safe.directory when using sudo
Date: Wed, 4 May 2022 06:02:54 -0700	[thread overview]
Message-ID: <CAPUEsphJrD5VUp+QiDFr+rp7MiRrPQO8vO++O-ibXZ+BhC43HQ@mail.gmail.com> (raw)
In-Reply-To: <d54b7672-36ab-a2b8-1a73-7d1a444a5936@gmail.com>

On Wed, May 4, 2022 at 4:15 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 03/05/2022 16:56, Carlo Marcelo Arenas Belón wrote:
> > On Tue, May 03, 2022 at 03:03:59PM +0100, Phillip Wood wrote:
> >>> +
> >>> +# this prerequisite should be added to all the tests, it not only prevents
> >>> +# the test from failing but also warms up any authentication cache sudo
> >>> +# might need to avoid asking for a password
> >>
> >> If this is required for all the tests then it would be simpler just to skip
> >> all the tests if it is not satisfied as you do above.
> >
> > it is obviously not required (as shown by some tests in patch 3 not having
> > it and by my choice if the word "should"),
>
> I'm afraid it is not obvious to me. As far as I can see the only test
> that does not have this prerequisite is 'cannot access if owned by root'
> added in patch 3. That test needs a setup test to run first which
> requires sudo so there is no point running it if this prerequisite is
> not satisfied.

We are in violent agreement here, but again (and don't take it wrong,
since it is most likely my fault for not being clear enough in my
request), the issue is I can't figure out how to make it obvious to
you since just the use of the world "should" made it obvious to me.

Do you have any suggestion I could include in a v4 reroll?, which will
be also sent as an RFC, so hopefully this time, we get at least
agreement in patches 1 and 2, so we could move forward and unblock the
development pipeline.

> > but it still recommended, which
> > I was hoping would be explained by that comment since if sudo to root is
> > only allowed "temporarily" by someone typing their password, then sudo keeps
> > that authentication in a cache, that could probably expire otherwise.
> >
> > Ironically, this comment was meant to explain why it was not checked once
> > at the beginning and being used instead in almost every test, but presume
> > I wasn't clear enough, not sure if worth rerolling either.
>
> That was not clear to me. Prerequisites are evaluated once and the
> result is cached.

This is indeed a bug, my intention was that it will be called in every
request so I need to at least make it "not lazy"

> Making it lazy just means it is evaluated when it is
> first required rather than when it is defined. You're right that we want
> to avoid sudo hanging because it is waiting for a password. We should
> define something like
>
> sudo () {
>         command sudo -n "$@"
> }

This gets us half way to what is needed.  Indeed I explicitly use sudo
-n in the "prerequisite" for this reason, and originally I used a perl
function that called sudo with a timeout and then killed it after a
fixed time.

The problem is we ALSO don't want the tests to fail if sudo suddenly
decides to ask for a password, so by design I wanted to detect that
issue in the prerequisite and disable the test instead, which I
obviously didn't get right.

> >> Running "sudo env" shows that it sets $HOME to /root which means that these
> >> tests will pick up /root/.gitconfig if it exists.
> >
> > I think this depends on how sudo is configured, but yes ANY environment
> > variables could be set to unsafe values that would confuse git if it assumes
> > it is still running as part of the test suite.
>
> I think I'm using the default configuration for that setting (or at
> least the default configured by the linux distribution I'm using).

As Junio pointed out in the discussion, this is likely not going to
run in a lot of places because of issues like that.
Indeed in Debian 10 (and therefore most likely all our Ubuntu based
CI) the prerequisite fails probably because of the same reason it
fails in your workstation.

sudo is configured to do '-H' by default (which we can't disable
unless we change the configuration AFAIK) and also doesn't run with
'-s' which means that even the following shows an error

  sudo command -v git

my hope was that it will at least run in the macOS part of the CI
which is better than nothing, or alternatively we could define an
alias as you suggested and force -s on it.

The problem of doing that though, is that then sudo might run the
wrong shell, which is also part of the reason why I was forcing it by
calling the script explicitly through the shell we want to use
instead.

> > My approach was to make sure (with the prerequisite) that at least we have
> > PATH set to the right value, so we won't start accidentally running the
> > system provided git, but you are correct that at least for patch1, the only
> > thing I can WARRANT to work is `git status`, but it should be also clear
> > to whoever writes tests using sudo, that it can't be otherwise since git it
> > is not only running as root, but it is running in the environment that sudo
> > provides when doing so.
> >
> >> Normally when running the
> >> tests we set $HOME to $TEST_DIRECTORY so they are run in a predictable
> >> environment. At least anything pointed to by core.hooksPath or
> >> core.fsmontior in that file is expecting to be run as root.
> >
> > which should be the same expectation of anyone running `sudo make install`
> > in their own repository, so we are just mimicking the use case we care
> > about.
>
> Two of the most important promises the suite makes are that (i) tests do
> not write outside $TEST_DIRECTORY and (ii) the tests are not affected by
> the user's or system's git config files. By having $HOME point to /root
> we are clearly violating the second promise and making it much easier to
> accidentally violate the first by inadvertently writing to $HOME.

While I think I'd seen my fair set of violations of those 2 principles
before, I agree that not violating them here would be a good option,
but I also consider this as part of the "integration with the
framework" that was explicitly punted.

Patch 1 only concerns with making an accurate reproduction of the
problem that was presented as a regression, so having the wrong shell
or having root's HOME if your sudo so prefers is by design what we
should do as well, and my ONLY warranty is that I would be able to
call the `git status` command I am trying to test by making sure that
at least sudo will (mostly) respect the PATH the test suite provides,
and which is also why I would rather have the prerequisite fail than
to make it work where it does not by default create a shell.

FWIW I don't think even that is perfect because a sufficiently evil
sudo configuration could force git to call a different status builtin,
but I thought calling the builtin directly by using git-status was
probably too much and also likely to fail in places where that binary
doesn't get created.

Patch 3 got what would be the beginnings of that "integration with the
framework", with an ugly and minimal "this is how we can inject
environment variables we care about" implementation that got of course
discarded in the RFC because it wasn't good enough and not strictly
needed.

I think your suggestion makes sense as an enhancement to patch3, but I
am not sure of how to get it done without reintroducing the
environment I got wrong already in the previous round.

> > core.hooksPath or core.fsmonitor might be relevant now, but there is no way
> > for me to predict what else might be in the future,
>
> exactly, they are just examples and show why setting HOME=root is a bad idea

I think that the current prerequisite prevents that already by failing
to work as I described before, is the prerequisite working on your
setup and still changing HOME?

this is what I get in macOS 11:

  $ sudo 'env' | grep HOME
  HOME=/Users/carlo
  $ sudo -H 'env' | grep HOME
  HOME=/var/root

> > and then again `sudo -H`
> > will behave differently than `sudo` and there is nothing git can do to
> > prevent that, so I keep thinking $HOME is not that special eitherway.
>
> I think $HOME is important enough to worry about because the test suite
> deliberately resets to avoid reading the user's config. Whether some
> other random variable such as GIT_COMMITTER_DATE is set or not does not
> matter in the same way.

I meant not important to our concerns that it will negatively impact
running these tests, I cannot provide any warranties that the sudo
environment provided wouldn't be evil enough, for example by setting
the path where git looks for its builtins.

but then again, I think that worrying about that is a stretch.

If root in a system we are running can change the sudoers
configurations or put configurations in root's home, that system has
more things to worry about than having this test running.

> > it might be worth adding that as well as a constraint into the prerequisite
> > though, so if your sudo does change HOME then we skip these tests, or we
> > try harder to call sudo in a way that doesn't change HOME instead.
>
> It would be better to call git via a wrapper that sets HOME correctly

I would rather make sure the prerequisite fails and all these tests
are skipped in that system.

Getting a wrapper that fixes THIS specific case won't protect against
many others

> >> I think it is
> >> worth spelling this out explicitly in the commit message (currently it is a
> >> bit vague about what the implications of not having better integration with
> >> the test framework are) and the top of the test file. Note that t1509
> >> sources test-lib.sh as the root user so does not have this issue.
> >
> > As explained before, there is no way to "explicitly" document all things that
> > might be relevant, and being vague was therefore by design.
>
> Being vague by design is unhelpful, just because it is difficult to list
> all the possible implications of a changes does not mean that one should
> not list the important known issues. Commit messages should be
> transparent about the known implications of the changes the commit
> introduces and whether there are likely to be other unanticipated
> implications.

fair enough, care to come with a suggestion?
again I think the "we are running things as root folks!!" is enough to
trigger a "better do not set that IKNOWWHATIAMDOING" variable on me,
but it might be my sysadmin experience talking.

> > t1509 has also a different objective AFAIK, which is to test in an environment
> > where everything is running as root, which is not what we want to do here.
>
> Indeed - I brought it up because we're reusing IKNOWWHATIAMDOING but not
> documenting that we using it in a different way.

It is not used in a different way.

IKNOWWHATIAMDOING is meant to keep developers from running potentially
dangerous stuff, that yes could mess with your system badly and which
also applies here.

BTW while trying to test this in CI, I realized it is not set there,
so might as well be changed to something different that will be and
that would ease your concerns.

> >>> +test_lazy_prereq SUDO '
> >>> +   sudo -n id -u >u &&
> >>> +   id -u root >r &&
> >>> +   test_cmp u r &&
> >>> +   command -v git >u &&
> >>> +   sudo command -v git >r &&
> >>> +   test_cmp u r
> >>> +'
> >>> +
> >>> +test_expect_success SUDO 'setup' '
> >>> +   sudo rm -rf root &&
> >>> +   mkdir -p root/r &&
> >>> +   sudo chown root root &&
> >>> +   (
> >>> +           cd root/r &&
> >>> +           git init
> >>
> >> Using git -C <directory> would eliminate a lot of the sub shells in this
> >> file
> >
> > My assumption (and help me understand if it was incorrect) is that these
> > tests should document the expected use cases, so you are correct that
> > both cd and -C accomplish the same in the end, but I think that cd is what
> > users would more normally use, and by writing with it (specially since it
> > requires a subshell) is also more easy to spot and understand that an
> > invocation of git with -C.
> >
> > I have to admit I didn't even thought of using -C originally because of
> > that, but if you think that makes the test easier to understand and better
> > I am sure happy to include that in a reroll.
>
> I think it's pretty common to use -C in the test suite when running git
> in a repository that is a subdirectory of $TEST_DIRECTORY.

I get that, but do you think that in this case makes the tests simpler
and more importantly more recognizable?

I'll try to use it more in the reroll, but examples where it actually
improve the tests would be useful.

Carlo

  reply	other threads:[~2022-05-04 13:04 UTC|newest]

Thread overview: 170+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-26 18:31 [RFC PATCH] git-compat-util: avoid failing dir ownership checks if running priviledged Carlo Marcelo Arenas Belón
2022-04-26 19:48 ` Derrick Stolee
2022-04-26 19:56   ` Junio C Hamano
2022-04-26 20:10     ` rsbecker
2022-04-26 20:45       ` Carlo Arenas
2022-04-26 21:10         ` Junio C Hamano
2022-04-26 20:12     ` Carlo Arenas
2022-04-26 20:26   ` Carlo Arenas
2022-04-29 16:16   ` Derrick Stolee
2022-04-27  0:05 ` [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-04-27  9:33   ` Phillip Wood
2022-04-27 12:30     ` Phillip Wood
2022-04-27 14:15       ` rsbecker
2022-04-27 15:58       ` Carlo Arenas
2022-04-27 16:14         ` Phillip Wood
2022-04-27 18:54           ` Junio C Hamano
2022-04-27 20:59             ` Carlo Arenas
2022-04-27 21:09               ` rsbecker
2022-04-27 21:25               ` Junio C Hamano
2022-04-28 17:56             ` Phillip Wood
2022-04-27 15:38     ` Carlo Arenas
2022-04-27 15:50       ` rsbecker
2022-04-27 16:19       ` Junio C Hamano
2022-04-27 16:45         ` Carlo Arenas
2022-04-27 17:22         ` Phillip Wood
2022-04-27 17:49           ` rsbecker
2022-04-27 17:54             ` Carlo Arenas
2022-04-27 18:05               ` rsbecker
2022-04-27 18:11                 ` Carlo Arenas
2022-04-27 18:16                   ` rsbecker
2022-04-27 16:31       ` Phillip Wood
2022-04-27 16:54         ` Carlo Arenas
2022-04-27 17:28           ` Phillip Wood
2022-04-27 17:49             ` Carlo Arenas
2022-04-27 22:26   ` [RFC PATCH v2] " Carlo Marcelo Arenas Belón
2022-04-27 22:33     ` Junio C Hamano
2022-04-28  3:35     ` [PATCH 0/2] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-04-28  3:35       ` [PATCH 1/2] Documentation: explain how safe.directory works when running under sudo Carlo Marcelo Arenas Belón
2022-04-28  5:17         ` Junio C Hamano
2022-04-28  5:58           ` Carlo Arenas
2022-04-28  6:41             ` Junio C Hamano
2022-04-28  3:35       ` [PATCH 2/2] t: add tests for safe.directory when running with sudo Carlo Marcelo Arenas Belón
2022-04-28  5:34         ` Junio C Hamano
2022-04-28  4:57       ` [PATCH 0/2] fix `sudo make install` regression in maint Junio C Hamano
2022-04-28 10:58       ` [PATCH v2 0/3] " Carlo Marcelo Arenas Belón
2022-04-28 10:58         ` [PATCH v2 1/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-04-28 18:02           ` Phillip Wood
2022-04-28 18:57             ` Carlo Arenas
2022-04-28 10:58         ` [PATCH v2 2/3] Documentation: explain how safe.directory works when running under sudo Carlo Marcelo Arenas Belón
2022-04-30  6:17           ` Bagas Sanjaya
2022-04-30  6:39             ` Junio C Hamano
2022-04-30 14:15             ` Carlo Marcelo Arenas Belón
2022-04-28 10:58         ` [PATCH v2 3/3] t: add tests for safe.directory when running with sudo Carlo Marcelo Arenas Belón
2022-04-28 16:55           ` Junio C Hamano
2022-04-28 18:08             ` Phillip Wood
2022-04-28 18:12               ` Junio C Hamano
2022-05-06 17:50                 ` Carlo Arenas
2022-05-06 21:43                   ` Junio C Hamano
2022-05-06 22:57                     ` Carlo Arenas
2022-05-06 23:55                       ` Junio C Hamano
2022-05-07 11:57                         ` Carlo Marcelo Arenas Belón
2022-04-28 19:53             ` rsbecker
2022-04-28 20:22               ` Carlo Arenas
2022-04-28 20:43                 ` rsbecker
2022-04-28 20:51                   ` Junio C Hamano
2022-04-28 20:56                   ` Carlo Arenas
2022-04-28 21:55                     ` rsbecker
2022-04-28 22:21                       ` Junio C Hamano
2022-04-28 22:45                         ` rsbecker
2022-04-28 20:46                 ` Junio C Hamano
2022-04-28 20:32               ` Junio C Hamano
2022-04-28 20:40                 ` rsbecker
2022-04-28 20:48                 ` Carlo Arenas
2022-04-28 21:02             ` Carlo Arenas
2022-04-28 21:07               ` Junio C Hamano
2022-04-29  1:24                 ` Carlo Marcelo Arenas Belón
2022-04-29 18:50                   ` Junio C Hamano
2022-04-29 20:05                     ` Carlo Marcelo Arenas Belón
2022-05-02 18:39         ` [RFC PATCH v3 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-02 18:39           ` [RFC PATCH v3 1/3] t: document regression git safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-02 21:35             ` Junio C Hamano
2022-05-02 23:07               ` Carlo Arenas
2022-05-02 18:39           ` [RFC PATCH v3 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-02 18:39           ` [RFC PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo Carlo Marcelo Arenas Belón
2022-05-02 22:10             ` Junio C Hamano
2022-05-03  0:00               ` Carlo Arenas
2022-05-03  6:54         ` [PATCH v3 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-03  6:54           ` [PATCH v3 1/3] t: document regression git safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-03 14:03             ` Phillip Wood
2022-05-03 15:56               ` Carlo Marcelo Arenas Belón
2022-05-04 11:15                 ` Phillip Wood
2022-05-04 13:02                   ` Carlo Arenas [this message]
2022-05-04 14:11                     ` Phillip Wood
2022-05-05 13:44             ` Johannes Schindelin
2022-05-05 14:34               ` Phillip Wood
2022-05-05 15:50               ` Junio C Hamano
2022-05-05 18:33               ` Junio C Hamano
2022-05-05 19:39                 ` Junio C Hamano
2022-05-06 21:03                   ` Carlo Arenas
2022-05-09  8:21                 ` Phillip Wood
2022-05-09 14:51                   ` Carlo Arenas
2022-05-09 15:18                     ` Phillip Wood
2022-05-09 16:01                   ` Junio C Hamano
2022-05-09 16:21                     ` Carlo Arenas
2022-05-06 17:39               ` Carlo Arenas
2022-05-03  6:54           ` [PATCH v3 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-05 14:01             ` Johannes Schindelin
2022-05-05 14:32               ` Phillip Wood
2022-05-06 19:15                 ` Carlo Arenas
2022-05-06 20:00                   ` Junio C Hamano
2022-05-06 20:22                     ` Carlo Arenas
2022-05-06 20:59                       ` Junio C Hamano
2022-05-06 21:40                         ` Carlo Arenas
2022-05-06 21:07                       ` rsbecker
2022-05-05 16:09               ` Junio C Hamano
2022-05-06 20:02               ` Carlo Arenas
2022-05-03  6:54           ` [PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo Carlo Marcelo Arenas Belón
2022-05-03 14:12             ` Phillip Wood
2022-05-03 15:27               ` Junio C Hamano
2022-05-06 16:54               ` Carlo Arenas
2022-05-07 16:35           ` [RFC PATCH v4 0/3] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-07 16:35             ` [RFC PATCH v4 1/3] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-07 16:35             ` [RFC PATCH v4 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-07 17:34               ` Junio C Hamano
2022-05-07 18:56                 ` Carlo Marcelo Arenas Belón
2022-05-09 16:54                   ` Junio C Hamano
2022-05-09 17:36                     ` rsbecker
2022-05-09 18:48                     ` Carlo Arenas
2022-05-09 19:16                       ` rsbecker
2022-05-09 19:41                       ` Junio C Hamano
2022-05-07 16:35             ` [RFC PATCH v4 3/3] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-10 14:17             ` [RFC PATCH v4 0/3] fix `sudo make install` regression in maint Phillip Wood
2022-05-10 15:47               ` Carlo Arenas
2022-05-10 17:46             ` [PATCH " Carlo Marcelo Arenas Belón
2022-05-10 17:46               ` [PATCH v4 1/3] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-05-10 22:10                 ` Junio C Hamano
2022-05-10 23:11                   ` Carlo Arenas
2022-05-10 23:44                     ` Junio C Hamano
2022-05-11  0:56                       ` Carlo Arenas
2022-05-11  1:11                         ` Junio C Hamano
2022-05-10 17:46               ` [PATCH v4 2/3] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-05-10 22:57                 ` Junio C Hamano
2022-05-11  7:34                   ` Carlo Arenas
2022-05-11 14:58                     ` Junio C Hamano
2022-05-10 17:46               ` [PATCH v4 3/3] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-10 23:11                 ` Junio C Hamano
2022-05-10 23:25                   ` Junio C Hamano
2022-05-11 14:04                   ` Carlo Arenas
2022-05-11 15:29                     ` Junio C Hamano
2022-05-13  1:00               ` [PATCH v5 0/4] fix `sudo make install` regression in maint Carlo Marcelo Arenas Belón
2022-05-13  1:00                 ` [PATCH v5 1/4] t: regression git needs safe.directory when using sudo Carlo Marcelo Arenas Belón
2022-06-03 12:12                   ` SZEDER Gábor
2022-05-13  1:00                 ` [PATCH v5 2/4] git-compat-util: avoid failing dir ownership checks if running privileged Carlo Marcelo Arenas Belón
2022-06-03 11:05                   ` SZEDER Gábor
2022-06-03 16:54                     ` Junio C Hamano
2022-06-03 17:34                       ` SZEDER Gábor
2022-05-13  1:00                 ` [PATCH v5 3/4] t0034: add negative tests and allow git init to mostly work under sudo Carlo Marcelo Arenas Belón
2022-05-13  1:20                   ` Junio C Hamano
2022-05-14 14:36                     ` Carlo Arenas
2022-05-15 16:54                       ` Junio C Hamano
2022-05-15 19:21                         ` Carlo Arenas
2022-05-16  5:27                           ` Junio C Hamano
2022-05-16 13:07                             ` Carlo Marcelo Arenas Belón
2022-05-16 16:25                               ` Junio C Hamano
2022-05-13  1:00                 ` [PATCH v5 4/4] git-compat-util: allow root to access both SUDO_UID and root owned Carlo Marcelo Arenas Belón
2022-06-15 14:02                   ` Johannes Schindelin
2022-06-17 14:26                     ` Carlo Arenas
2022-06-17 16:00                       ` Junio C Hamano
2022-06-17 20:23                   ` [PATCH v6] " Carlo Marcelo Arenas Belón
2022-06-17 21:02                     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPUEsphJrD5VUp+QiDFr+rp7MiRrPQO8vO++O-ibXZ+BhC43HQ@mail.gmail.com \
    --to=carenas@gmail.com \
    --cc=bagasdotme@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCH v3 1/3] t: document regression git safe.directory when using sudo' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).