| Message ID | 20220513010020.55361-5-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix `sudo make install` regression in maint | expand |
Hi Carlo, On Thu, 12 May 2022, Carlo Marcelo Arenas Belón wrote: > diff --git a/git-compat-util.h b/git-compat-util.h > index e7cbfa65c9a..0a5a4ee7a9a 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -420,9 +420,10 @@ static inline int git_offset_1st_component(const char *path) > * maybe provide you with a patch that would prevent this issue again > * in the future. > */ > -static inline void extract_id_from_env(const char *env, uid_t *id) > +static inline int id_from_env_matches(const char *env, uid_t id) I agree somewhat with Gabór's concern that this patch tries to do too many things at once, including this rename. We have a recent history of introducing so many regressions that `.1` releases have become the norm rather than the exception, and from where I sit, the reason is squarely with the uptick in refactoring (including renames like this one). So unless the refactoring is done to any other end than for refactoring's own sake (which is really not a good reason), I see it as problematic. > { > const char *real_uid = getenv(env); > + int matches = 0; > > /* discard anything empty to avoid a more complex check below */ > if (real_uid && *real_uid) { > @@ -432,9 +433,10 @@ static inline void extract_id_from_env(const char *env, uid_t *id) > errno = 0; > /* silent overflow errors could trigger a bug here */ > env_id = strtoul(real_uid, &endptr, 10); > - if (!*endptr && !errno) > - *id = env_id; > + if (!*endptr && !errno && (uid_t)env_id == id) > + matches = 1; > } > + return matches; > } > > static inline int is_path_owned_by_current_uid(const char *path) > @@ -446,10 +448,13 @@ static inline int is_path_owned_by_current_uid(const char *path) > return 0; > > euid = geteuid(); > + if (st.st_uid == euid) > + return 1; > + > if (euid == ROOT_UID) > - extract_id_from_env("SUDO_UID", &euid); > + return id_from_env_matches("SUDO_UID", st.st_uid); A much shorter, much more obvious patch would look like this: - if (euid == ROOT_UID) + if (st.st_uid != euid && euid == ROOT_UID && ) extract_id_from_env("SUDO_UID", &euid); It accomplishes the same goal, but is eminently easier to review. For regression fixes, I much prefer the safety and confidence that comes with that. Ciao, Dscho
On Wed, Jun 15, 2022 at 7:03 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > On Thu, 12 May 2022, Carlo Marcelo Arenas Belón wrote: > > > diff --git a/git-compat-util.h b/git-compat-util.h > > index e7cbfa65c9a..0a5a4ee7a9a 100644 > > --- a/git-compat-util.h > > +++ b/git-compat-util.h > > @@ -420,9 +420,10 @@ static inline int git_offset_1st_component(const char *path) > > * maybe provide you with a patch that would prevent this issue again > > * in the future. > > */ > > -static inline void extract_id_from_env(const char *env, uid_t *id) > > +static inline int id_from_env_matches(const char *env, uid_t id) > > I agree somewhat with Gabór's concern that this patch tries to do too many > things at once, including this rename. > > We have a recent history of introducing so many regressions that `.1` > releases have become the norm rather than the exception, and from where I > sit, the reason is squarely with the uptick in refactoring (including > renames like this one). > > So unless the refactoring is done to any other end than for refactoring's > own sake (which is really not a good reason), I see it as problematic. I couldn't agree more with that sentiment, but in this case the refactoring was done to clean up the recently introduced function which was indeed too ugly (some would say on purpose) and replacing it with not only better looking code, but also a cleaner interface. I would think that in this specific case an exemption could be granted, but it is also true that while this code is almost a month old, it hasn't got the review it will require to be merged so late in the release cycle without raising the concerns you both fairly put forward. > > { > > const char *real_uid = getenv(env); > > + int matches = 0; > > > > /* discard anything empty to avoid a more complex check below */ > > if (real_uid && *real_uid) { > > @@ -432,9 +433,10 @@ static inline void extract_id_from_env(const char *env, uid_t *id) > > errno = 0; > > /* silent overflow errors could trigger a bug here */ > > env_id = strtoul(real_uid, &endptr, 10); > > - if (!*endptr && !errno) > > - *id = env_id; > > + if (!*endptr && !errno && (uid_t)env_id == id) > > + matches = 1; > > } > > + return matches; > > } > > > > static inline int is_path_owned_by_current_uid(const char *path) > > @@ -446,10 +448,13 @@ static inline int is_path_owned_by_current_uid(const char *path) > > return 0; > > > > euid = geteuid(); > > + if (st.st_uid == euid) > > + return 1; > > + > > if (euid == ROOT_UID) > > - extract_id_from_env("SUDO_UID", &euid); > > + return id_from_env_matches("SUDO_UID", st.st_uid); > > A much shorter, much more obvious patch would look like this: > > - if (euid == ROOT_UID) > + if (st.st_uid != euid && euid == ROOT_UID && ) > extract_id_from_env("SUDO_UID", &euid); > > It accomplishes the same goal, but is eminently easier to review. For > regression fixes, I much prefer the safety and confidence that comes with > that. will reroll with your suggestion, thanks again for your helpful review, and apologies again to everyone for not cleaning it up earlier in the cycle. IMHO the alternative (which will be releasing with the regression) might be worse than releasing with this version so we ought to do something before RC1. Carlo Carlo
Carlo Arenas <carenas@gmail.com> writes: >> So unless the refactoring is done to any other end than for refactoring's >> own sake (which is really not a good reason), I see it as problematic. > > I couldn't agree more with that sentiment, but in this case the > refactoring was done to clean up the recently introduced function > which was indeed too ugly (some would say on purpose) and replacing it > with not only better looking code, but also a cleaner interface. > > I would think that in this specific case an exemption could be > granted, but it is also true that while this code is almost a month > old, it hasn't got the review it will require to be merged so late in > the release cycle without raising the concerns you both fairly put > forward. > >> > if (euid == ROOT_UID) >> > - extract_id_from_env("SUDO_UID", &euid); >> > + return id_from_env_matches("SUDO_UID", st.st_uid); >> >> A much shorter, much more obvious patch would look like this: >> >> - if (euid == ROOT_UID) >> + if (st.st_uid != euid && euid == ROOT_UID && ) >> extract_id_from_env("SUDO_UID", &euid); >> >> It accomplishes the same goal, but is eminently easier to review. For >> regression fixes, I much prefer the safety and confidence that comes with >> that. > > will reroll with your suggestion, thanks again for your helpful > review, and apologies again to everyone for not cleaning it up earlier > in the cycle. As long as the syntax error with the "like this" suggestion gets fixed, I do not mind if you took the above verbatim ;-) Quite honestly, at this low level of small "the caller asks if it is safe for the current user to step into the given path to run git" function, it would be a waste of time to quibble about the separation of concerns between the helper function that figures out who the "current user" is, and checking it against the user who owns the path. It is not like we are talking about a brutal-to-reviewers 250-line function that is called from multiple places after all. The extract_id_from_env() helper could be inlined the caller and the issue would disappear (I am not making a suggestion to actually do so, but an illustration why it does not matter much in the larger picture for something miniscule like this). Where the function name and the separation of concerns truly matters is between the is_path_owned_by*() helper and its callers, as in the longer term I expect it won't be just "ownership" that we would care about and once we start checking permission bits and other things for the path, we'd regret the "owned by" part of the name. But that is not our immediate concern, so let's take whatever that is not incorrect and can achieve "concensus" the easiest and move on. Thanks for sticking with the larger topic of making the end-user experience better around here. Very much appreciated.
diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index c6ebd1674dd..3128b132713 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -31,11 +31,12 @@ As explained, Git only allows you to access repositories owned by yourself, i.e. the user who is running Git, by default. When Git is running as 'root' in a non Windows platform that provides sudo, however, git checks the SUDO_UID environment variable that sudo creates -and will allow access to the uid recorded as its value instead. +and will allow access to the uid recorded as its value in addition to +the id from 'root'. This is to make it easy to perform a common sequence during installation "make && sudo make install". A git process running under 'sudo' runs as 'root' but the 'sudo' command exports the environment variable to record which id the original user has. If that is not what you would prefer and want git to only trust -repositories that are owned by root instead, then you must remove +repositories that are owned by root instead, then you can remove the `SUDO_UID` variable from root's environment before invoking git. diff --git a/git-compat-util.h b/git-compat-util.h index e7cbfa65c9a..0a5a4ee7a9a 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -420,9 +420,10 @@ static inline int git_offset_1st_component(const char *path) * maybe provide you with a patch that would prevent this issue again * in the future. */ -static inline void extract_id_from_env(const char *env, uid_t *id) +static inline int id_from_env_matches(const char *env, uid_t id) { const char *real_uid = getenv(env); + int matches = 0; /* discard anything empty to avoid a more complex check below */ if (real_uid && *real_uid) { @@ -432,9 +433,10 @@ static inline void extract_id_from_env(const char *env, uid_t *id) errno = 0; /* silent overflow errors could trigger a bug here */ env_id = strtoul(real_uid, &endptr, 10); - if (!*endptr && !errno) - *id = env_id; + if (!*endptr && !errno && (uid_t)env_id == id) + matches = 1; } + return matches; } static inline int is_path_owned_by_current_uid(const char *path) @@ -446,10 +448,13 @@ static inline int is_path_owned_by_current_uid(const char *path) return 0; euid = geteuid(); + if (st.st_uid == euid) + return 1; + if (euid == ROOT_UID) - extract_id_from_env("SUDO_UID", &euid); + return id_from_env_matches("SUDO_UID", st.st_uid); - return st.st_uid == euid; + return 0; } #define is_path_owned_by_current_user is_path_owned_by_current_uid diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh index a621f1ea5eb..ff311761289 100755 --- a/t/t0034-root-safe-directory.sh +++ b/t/t0034-root-safe-directory.sh @@ -68,7 +68,7 @@ test_expect_success 'can access if addressed explicitly' ' ) ' -test_expect_failure SUDO 'can access with sudo if root' ' +test_expect_success SUDO 'can access with sudo if root' ' ( cd root/p && sudo git status @@ -85,19 +85,6 @@ test_expect_success SUDO 'can access with sudo if root by removing SUDO_UID' ' ) ' -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
Previous changes introduced a regression which will prevent root for accessing repositories owned by thyself if using sudo because SUDO_UID takes precedence. Loosen that restriction by allowing root to access repositories owned by both uid by default and without having to add a safe.directory exception. A previous workaround that was documented in the tests is no longer needed so it has been removed together with its specially crafted prerequisite. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Documentation/config/safe.txt | 5 +++-- git-compat-util.h | 15 ++++++++++----- t/t0034-root-safe-directory.sh | 15 +-------------- 3 files changed, 14 insertions(+), 21 deletions(-)