Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo

[Junio C Hamano <gitster@pobox.com>]

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).