[v4,0/3] fix `sudo make install` regression in maint

Message

Carlo Marcelo Arenas Belón May 10, 2022, 5:46 p.m. UTC

A reroll for cb/path-owner-check-with-sudo with all the suggestions by
Junio since RFC v4 and that only include a small code change in patch 2
to get rid of the maligned and probably buggy `(uid_t)-1` which is not
needed and making sure the code would most likely work (ad discussed)
even when uid_t is signed, sudo behaves and git is running as root.

Range-diff provided below to make easier to spot all the other changes
in documentation and commit messages as well.

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              |  54 ++++++++++++++++-
 t/lib-sudo.sh                  |  12 ++++
 t/t0034-root-safe-directory.sh | 103 +++++++++++++++++++++++++++++++++
 4 files changed, 178 insertions(+), 1 deletion(-)
 create mode 100644 t/lib-sudo.sh
 create mode 100755 t/t0034-root-safe-directory.sh

--
1:  4d078785cb ! 1:  8296d93ec0 t: regression git needs safe.directory when using sudo
    @@ Commit message
     
         The test file also uses at least one initial "setup" test that creates
         a parallel execution directory, while ignoring the repository created
    -    by the test framework, and special care should be taken when invoking
    -    commands through sudo, since the environment is otherwise independent
    -    from what the test framework expects.  Indeed `git status` was used
    -    as a proxy because it doesn't even require commits in the repository
    -    to work.
    +    by the test framework.
    +
    +    Special care should be taken when invoking commands through sudo, since
    +    the environment is otherwise independent from what the test framework
    +    setup and might have changed the values for HOME, SHELL and dropped
    +    several relevant environment variables for your test.  Indeed `git status`
    +    was used as a proxy because it doesn't even require commits in the
    +    repository to work and usually doesn't require much from the environment
    +    to run, but a future patch will add calls to `git init` and that will
    +    fail to honor the default branch name, unless that setting is NOT
    +    provided through an environment variable (which means even a CI run
    +    could fail that test if enabled incorrectly).
     
         A new SUDO prerequisite is provided that does some sanity checking
         to make sure the sudo command that will be used allows for passwordless
    @@ Commit message
         execution path.  This matches what is provided by the macOS agents that
         are used as part of GitHub actions and probably nowhere else.
     
    -    Most of those characteristics make this test mostly suitable only for
    +    Most of those characteristics make this test mostly only suitable for
         CI, but it might be executed locally if special care is taken to provide
    -    for some of them in the local configuration and maybe making use of the
    +    for all of them in the local configuration and maybe making use of the
         sudo credential cache by first invoking sudo, entering your password if
         needed, and then invoking the test with:
     
           $ GIT_TEST_ALLOW_SUDO=YES ./t0034-root-safe-directory.sh
     
         If it fails to run, then it means your local setup wouldn't work for the
    -    test and things that might help is to comment out sudo's secure_path config
    -    and make sure your account has similar privileges than what the CI
    -    provides (for example an entry in /etc/sudoers for the user marta like)
    +    test because of the configuration sudo has or other system settings, and
    +    things that might help are to comment out sudo's secure_path config, and
    +    make sure that the account you are using has no restrictions on the
    +    commands it can run through sudo, just like is provided for the user in
    +    the CI.
    +
    +    For example (assuming a username of marta for you) something probably
    +    similar to the following entry in your /etc/sudoers (or equivalent) file:
     
           marta ALL=(ALL:ALL) NOPASSWD: ALL
     
         Reported-by: SZEDER Gábor <[e-mail]>
         Helped-by: Phillip Wood <[e-mail]>
         Signed-off-by: Carlo Marcelo Arenas Belón <[e-mail]>
    -    Signed-off-by: Junio C Hamano <[e-mail]>
     
      ## t/t0034-root-safe-directory.sh (new) ##
     @@
2:  073b73eb26 ! 2:  e3efd6a178 git-compat-util: avoid failing dir ownership checks if running privileged
    @@ Commit message
         privileges didn't track the original id in a sudo compatible way.
     
         Because of compatibility with sudo, the code assumes that uid_t is an
    -    unsigned integer type, but adds additional logic to protect itself
    -    against possibly malicious ids outside the expected range and ignore
    -    them.
    -
    -    A warning should be generated if uid_t is signed and the code would
    -    need to be locally patched to work correctly, but this is also a
    -    weather balloon of sorts so we will then now which systems those are
    -    and whether we should accommodate for their portability in our codebase.
    +    unsigned integer type (which is not required by the standard) but is used
    +    that way in their codebase to generate SUDO_UID.  In systems where uid_t
    +    is signed, sudo might be also patched to NOT be unsigned and that might
    +    be able to trigger an edge case and a bug (as described in the code), but
    +    it is considered unlikely to be triggered, and even when it does the code
    +    would just safely fail, so there is no attempt either to detect it or
    +    prevent it in the code, which might need to be changed in the future.
     
         Reported-by: Guy Maurel <[e-mail]>
         Helped-by: SZEDER Gábor <[e-mail]>
    @@ Commit message
         Helped-by: Phillip Wood <[e-mail]>
         Suggested-by: Johannes Schindelin <[e-mail]>
         Signed-off-by: Carlo Marcelo Arenas Belón <[e-mail]>
    -    Signed-off-by: Junio C Hamano <[e-mail]>
     
      ## Documentation/config/safe.txt ##
     @@ Documentation/config/safe.txt: directory was listed in the `safe.directory` list. If `safe.directory=*`
    @@ Documentation/config/safe.txt: directory was listed in the `safe.directory` list
     +obviously do so with the uid of the user that is running git itself,
     +but if git is running as root, in a platform that provides sudo and is
     +not Windows, it will check first if it might have been started through
    -+it, and if that is the case, will instead use the uid of the user that
    -+did invoke that instead.
    ++it, and if that is the case, will use the uid of the user that invoked
    ++sudo instead.
     +If that is not what you would prefer and want git to only trust
     +repositories that are owned by root instead, then you should remove
     +the `SUDO_UID` variable from root's environment.
    @@ git-compat-util.h: static inline int git_offset_1st_component(const char *path)
     +#endif
     +
     +/*
    -+ * this helper function overrides a ROOT_UID with the one provided by
    ++ * This helper function overrides a ROOT_UID with the one provided by
     + * an environment variable, do not use unless the original user is
    -+ * root
    -+ * WARNING: this function assumes uid_t is unsigned, if you got here
    -+ *          because of a warning or a bug will need a patch and would
    -+ *          be nice if you let us know
    ++ * root or could be used as means to escalate to root privileges.
    ++ *
    ++ * PORTABILITY WARNING:
    ++ * This code assumes uid_t is unsigned because that is what sudo does.
    ++ * If your uid_t type is signed and all your ids are positive then it
    ++ * should all work fine, but need to make sure sudo never generates a
    ++ * value higher than what can be represented by your uid_t type or a
    ++ * negative value or it will trigger wraparound bugs.
    ++ *
    ++ * If that happens the uid used might be incorrect and then trigger
    ++ * an access error from the filesystem itself.
    ++ *
    ++ * In the unlikely scenario this happened to you, and that is how you
    ++ * got to this message, we would like to know about it by letting us
    ++ * now with an email to [e-mail] indicating which platform,
    ++ * you are running on and which version of sudo you used to see if we
    ++ * can provide you a patch that would prevent this issue in the future.
     + */
     +static inline void extract_id_from_env(const char *env, uid_t *id)
     +{
     +	const char *real_uid = getenv(env);
     +
    -+	/* discard any empty values */
    ++	/* discard anything empty to avoid a more complex check below */
     +	if (real_uid && *real_uid) {
     +		char *endptr = NULL;
     +		unsigned long env_id;
     +
     +		errno = 0;
    ++		/* silent overflow errors could trigger a bug below */
     +		env_id = strtoul(real_uid, &endptr, 10);
    -+		/*
    -+		 * env_id could underflow/overflow in the previous call
    -+		 * and if it will still fit in a long it will not report
    -+		 * it as error with ERANGE, instead silently using an
    -+		 * equivalent positive number that might be bogus.
    -+		 * if uid_t is narrower than long, it might not fit,
    -+		 * hence why we  need to check it against the maximum
    -+		 * possible uid_t value before accepting it.
    -+		 */
    -+		if (!*endptr && !errno && env_id <= (uid_t)-1)
    ++		if (!*endptr && !errno)
     +			*id = env_id;
     +	}
     +}
3:  e772e4d128 ! 3:  1b8003c599 t0034: add negative tests and allow git init to mostly work under sudo
    @@ Commit message
     
         Note that the specific test that documents that after the previous
         changes, it is no longer possible for root (if obtained through sudo)
    -    to NOT add an exception or need a "workaround" to be able to run git
    +    to NOT add an exception or NOT need a "workaround" to be able to run git
         commands in a repository owned by thyself, is marked as a regression
         and is expected to be fixed with a future change, which hasn't been
         provided yet and that is not part of this series.
     
    +    As described in the comments from the test itself, at least one of the
    +    documented workarounds could fail if sudo doesn't allow root to call sudo
    +    or if root is not in sudoers, but that is very unlikely, and more
    +    importantly actually not what is provided by the currently supported sudo
    +    configuration this test already relies on and therefore adding a way to
    +    validate it works in the prerequisite has been punted for now.
    +
         Helped-by: Junio C Hamano <[e-mail]>
         Helped-by: Phillip Wood <[e-mail]>
         Signed-off-by: Carlo Marcelo Arenas Belón <[e-mail]>
    -    Signed-off-by: Junio C Hamano <[e-mail]>
     
      ## t/lib-sudo.sh (new) ##
     @@
    @@ t/t0034-root-safe-directory.sh: test_expect_success SUDO 'sudo git status as ori
     +'
     +
     +test_expect_success SUDO 'can access with sudo using a workaround' '
    -+	# run sudo twice; would fail is root is not in the sudoers
    ++	# run sudo twice; would fail if root is not in sudoers
     +	(
     +		cd root/p &&
     +		sudo sudo git status