| Message ID | 20220507163508.78459-1-carenas@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix `sudo make install` regression in maint | expand |
Hi Carlo On 07/05/2022 17:35, Carlo Marcelo Arenas Belón wrote: > A reroll for cb/path-owner-check-with-sudo with most of the suggestions > included but planned originally as an RFC, because I am frankly not sure > that I read and addressed all of it, but also because after seeing how > my only chance to get an official Reviewed-by: Junio vanished I also > realized that I wasn't been clear enough and careful enough from the > beginning to explain the code correctly and therefore had maybe wasted > more of the review quota this change should require. > > Important changes (eventhough most are not affecting the logic) > * Document hopefully better which environments are supported and what > to do if you want to test it in one that is not (thanks to dscho) > * Removed the arguably premature optimization to try to keep the sudo > cache warm which was actually buggy, as it was also not needed. > The CI does passwordless sudo unconditionally and even when running > it locally it will go so fast that is shouldn't be an issue (thanks > to Philip) > * No longer using the ugly and controversial variable name so now it > will need GIT_TEST_ENABLE_SUDO to be used to enable it on CI (which > is not done in this series, but a run with it enabled on top of > seen is available[1]) > * Stop the arguing about what is or not a regression worth fixing and > instead document it as one through a test, which would be easy to > fix in a follow up since the code was already provided by Junio I've had a read of the patches and I agree with Junio's comments on the second patch. I'd really like us to avoid sudo in the tests if we can as it causes a lot of problems. Just to let you know I'm going to be off the list for the next couple of weeks, so I wont be looking at these patches in that time. Best Wishes Phillip > Lastly I am little concerned by the fact this is going to maint but > has a "weather balloon" of sorts, which might not be wise, since it > might prevent people that might be affected from upgrading if they > have a -Werror policy. > > The effect is minor though, as worst case, if someone has a system > with a signed uid_t then this "feature" wouldn't work for them and > nothing has changed but I think it is worth to consider the alternatives > which are (in my own order of preference) > > * Revert the change to use unsigned long and strtoul() > > This will mean that people running in a 32bit system with an uid bigger > than INT_MAX wouldn't be able to use the feature > > * Move the code out (which is indeed an artificial restriction) so that > we can use intmax_t and strtoimax() instead and a cast to compare the > uid_t. > > This avoids all issues and restrictions but means more code changes > > * Throw away the custom function and expand the API ones to be used > instead as dscho suggested. > > Even more code changes, but maybe less risk as we will be building > on top of battle tested code. > > [1] https://github.com/carenas/git/actions/runs/2286452160 > > Carlo Marcelo Arenas Belón (3): > t: regression git needs safe.directory when using sudo > git-compat-util: avoid failing dir ownership checks if running > privileged > t0034: add negative tests and allow git init to mostly work under sudo > > Documentation/config/safe.txt | 10 ++++ > git-compat-util.h | 49 +++++++++++++++- > t/lib-sudo.sh | 12 ++++ > t/t0034-root-safe-directory.sh | 103 +++++++++++++++++++++++++++++++++ > 4 files changed, 173 insertions(+), 1 deletion(-) > create mode 100644 t/lib-sudo.sh > create mode 100755 t/t0034-root-safe-directory.sh >
On Tue, May 10, 2022 at 7:17 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 07/05/2022 17:35, Carlo Marcelo Arenas Belón wrote: > > A reroll for cb/path-owner-check-with-sudo with most of the suggestions > > included but planned originally as an RFC, because I am frankly not sure > > that I read and addressed all of it, but also because after seeing how > > my only chance to get an official Reviewed-by: Junio vanished I also > > realized that I wasn't clear enough and careful enough from the > > beginning to explain the code correctly and therefore had maybe wasted > > more of the review quota this change should require. > > > > Important changes (eventhough most are not affecting the logic) > > * Document hopefully better which environments are supported and what > > to do if you want to test it in one that is not (thanks to dscho) > > * Removed the arguably premature optimization to try to keep the sudo > > cache warm which was actually buggy, as it was also not needed. > > The CI does passwordless sudo unconditionally and even when running > > it locally it will go so fast that is shouldn't be an issue (thanks > > to Phillip) > > * No longer using the ugly and controversial variable name so now it > > will need GIT_TEST_ENABLE_SUDO to be used to enable it on CI (which > > is not done in this series, but a run with it enabled on top of > > seen is available[1]) > > * Stop the arguing about what is or not a regression worth fixing and > > instead document it as one through a test, which would be easy to > > fix in a follow up since the code was already provided by Junio > > I've had a read of the patches and I agree with Junio's comments on the > second patch. Not sure which comments you are referring to, but I'd implemented Junio's suggestion and removed the "too clever" (uid_t)-1 in v4. > I'd really like us to avoid sudo in the tests if we can as > it causes a lot of problems. Even if it might not seem like it, I agree with you both (and dscho) too, and if I could come out with a way to do so that would be still secure, I would do it in a pinch, but I can't (at least until now) and I don't want for that to hold up this fix so will be publishing soon a v4 that still uses sudo in the tests, I am afraid. > Just to let you know I'm going to be off > the list for the next couple of weeks, so I wont be looking at these > patches in that time. Thanks for all your help reviewing them and more importantly improving them, enjoy your time off. Carlo
A reroll for cb/path-owner-check-with-sudo with most of the suggestions included but planned originally as an RFC, because I am frankly not sure that I read and addressed all of it, but also because after seeing how my only chance to get an official Reviewed-by: Junio vanished I also realized that I wasn't been clear enough and careful enough from the beginning to explain the code correctly and therefore had maybe wasted more of the review quota this change should require. Important changes (eventhough most are not affecting the logic) * Document hopefully better which environments are supported and what to do if you want to test it in one that is not (thanks to dscho) * Removed the arguably premature optimization to try to keep the sudo cache warm which was actually buggy, as it was also not needed. The CI does passwordless sudo unconditionally and even when running it locally it will go so fast that is shouldn't be an issue (thanks to Philip) * No longer using the ugly and controversial variable name so now it will need GIT_TEST_ENABLE_SUDO to be used to enable it on CI (which is not done in this series, but a run with it enabled on top of seen is available[1]) * Stop the arguing about what is or not a regression worth fixing and instead document it as one through a test, which would be easy to fix in a follow up since the code was already provided by Junio Lastly I am little concerned by the fact this is going to maint but has a "weather balloon" of sorts, which might not be wise, since it might prevent people that might be affected from upgrading if they have a -Werror policy. The effect is minor though, as worst case, if someone has a system with a signed uid_t then this "feature" wouldn't work for them and nothing has changed but I think it is worth to consider the alternatives which are (in my own order of preference) * Revert the change to use unsigned long and strtoul() This will mean that people running in a 32bit system with an uid bigger than INT_MAX wouldn't be able to use the feature * Move the code out (which is indeed an artificial restriction) so that we can use intmax_t and strtoimax() instead and a cast to compare the uid_t. This avoids all issues and restrictions but means more code changes * Throw away the custom function and expand the API ones to be used instead as dscho suggested. Even more code changes, but maybe less risk as we will be building on top of battle tested code. [1] https://github.com/carenas/git/actions/runs/2286452160 Carlo Marcelo Arenas Belón (3): t: regression git needs safe.directory when using sudo git-compat-util: avoid failing dir ownership checks if running privileged t0034: add negative tests and allow git init to mostly work under sudo Documentation/config/safe.txt | 10 ++++ git-compat-util.h | 49 +++++++++++++++- t/lib-sudo.sh | 12 ++++ t/t0034-root-safe-directory.sh | 103 +++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 1 deletion(-) create mode 100644 t/lib-sudo.sh create mode 100755 t/t0034-root-safe-directory.sh