Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 1, 2024, 16:34 UTC
Message-ID: <ae658244-877f-c5cf-8947-83b87b66d01f@gmx.de>
In-Reply-To: <72e42e9f-5b85-4863-8506-c99d658d7596@gmail.com>

Hi Phillip,

On Wed, 26 Jun 2024, Phillip Wood wrote:

Show 22 quoted lines

> On 26/06/2024 14:11, Phillip Wood wrote:
> > Hi Florian
> >
> > On 26/06/2024 13:33, Florian Schmaus wrote:
> > > Sometimes more flexibility to disable/ignore the ownership check, besides
> > > the safe.directory configuration option, is required.
> > >
> > > For example, git-daemon running as nobody user, which typically has no
> > > home directory. Therefore, we can not add the path to a user-global
> > > configuration and adding the path to the system-wide configuration could
> > > have negative security implications.
> > >
> > > Therefore, make the check configurable via an environment variable.
> >
> > An alternative would be to allow safe.directory to be specified on the
> > command line with "git -c safe.directory='*' daemon ..." rather than adding
> > a dedicated environment variable.
>
> To expand an this a little - a couple of times I've wanted to checkout a bare
> repository that is owned by a different user. It is a pain to have to add a
> new config setting just for a one-off checkout. Being able to adjust the
> config on the command line would be very useful in that case.

It is somewhat surprising that this `-c safe.directory=*` method does _not_ work for local clones. To verify, I ran this:

  git init --bare other-user.git &&
  sudo chown -R 9999.9999 other-user.git/ &&
  git -c safe.directory=\* clone other-user.git/

This will complain about the dubious ownership and suggest to add the `safe.directory` setting to the user-wide config, ignoring the command-line config altogether.

The reason is to be found in https://github.com/git/git/blob/v2.45.2/connect.c#L1462-L1464:

		/* remove repo-local variables from the environment */
		for (var = local_repo_env; *var; var++)
			strvec_push(&conn->env, *var);

The `local_repo_env` array _specifically_ lists `GIT_CONFIG_PARAMETERS` in https://github.com/git/git/blob/v2.45.2/environment.c#L129 to be removed from the environment when spawning the `git upload-pack` process.

It was not originally listed, but added via https://lore.kernel.org/git/20100824064114.GA20724@burratino/, where the commit message does not really shed light into the question why this was desirable, and there is no discussion in that mail thread about this aspect of the patch, but at least the added test case reveals the intention in some sort of way: The `-c` option allows to specify `receive.denyDeletes`, and in the described scenario the idea was that it would only apply to the client side of a local `receive-pack` but not the "remote" one. As the example above illustrates, that patch might have been overly focused on one specific, particular scenario.

Ciao, Johannes