Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 28, 2024, 09:35 UTC
Message-ID: <27cadffb-ca3f-487d-86b7-3508c45c446d@gmail.com>
In-Reply-To: <xmqqpls2v1zx.fsf@gitster.g>

On 27/06/2024 16:28, Junio C Hamano wrote:

Show 28 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> On 26/06/2024 19:11, Junio C Hamano wrote:
>>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>>
>>>> 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.
>>> True.  As long as it is deemed safe to honor the one-off "git -c
>>> safe.directory=..." from the command line, for the purpose of this
>>> "I who am running this 'git' process hereby declare that I trust
>>> this and that repository", I think it would be the best solution
>>> for the "git daemon" use case.
>>
>> This actually works already, the behavior was changed in 6061601d9f
>> (safe.directory: use git_protected_config(), 2022-07-14). The reason I
>> thought it didn't work was that I remember it failing on Debian
>> bullseye a few months ago but that used an older version of git. There
>> is some more rationale for the change in 779ea9303a7 (Documentation:
>> define protected configuration, 2022-07-14)
> 
> Thanks.
> 
> So, does this more or less conclude the episode about how best to
> deal with the 2.45.1 regression that Florian's patch in this thread
> started?

I think so yes

Show 13 quoted lines

> It seems that we already have enough mechanisms to help
> users tweak their existing set-up, so we may not need code changes,
> but I am wondering if we want to add a bit of documentation around
> safe.directory to tell them when it makes sense to set it, what
> value(s) they would want to set it to, etc.
> 
>   * For "git daemon" invocations, because we know the command is run
>     after chdir to a directory with '.' specified as the repository,
>     we recommend to have safe.directory=., either on the command line
>     with "-c var=val" or in daemon user's ~/.gitconfig, in the
>     "git-daemon" help page?  We could recommend safe.directory=*, but
>     they would mean the same thing in the context of running "git
>     daemon".

I think we'd be better to fix the safe.directory check as you suggest below if we can but failing that updating the documentation would certainly help.

Show 5 quoted lines

>     We may want to discuss who protects from whom with the
>     safe.directory mechanism and git-daemon-export-ok mechanism.  The
>     former is "the daemon trusts that repositories won't harm the
>     daemon user", while the latter is "the repository owner is OK for
>     it to be published".

Yes that would be helpful

>     Also optionally, we may update the code to take the absolute path
>     of the repository before passing it to the safe.directory check.

I think doing this would be more helpful than updating the documentation to recommend adding "safe.directory=.". If we do this we would also want to convert "//" -> "/" in the config keys as we've been forcing users to add paths like "/srv/git//my-repo" if the --base-path argument to git-daemon ended with a "/"

>   * For "http-backend" invocations, we should think about potential
>     additions that would help users, similar to what I listed above
>     for "git daemon".

That sounds sensible.

Show 6 quoted lines

> Having said all that, I do not think I mind GIT_SAFE_DIRECTORIES
> that is a ":" separated list of paths that is honored just like the
> multi-valued configuration variable safe.directory.  Once an
> attacker can influence your environment variables, it already is
> game over, so trusting it does not make the attack surface any
> worse.

Indeed in that case the attacker can influence the path that we read the protected config from by setting $HOME (and do far worse by setting $PATH)

Show 6 quoted lines

> As Peff explained, we can trigger the more general "git -c
> var=val" mechanism by exporting a set of environment variables, so
> such a specialized environment variable is not strictly needed, but
> it would make writing the "SetEnv" directive in apache configuration
> (and similar ones for other HTTP server implementations) slighly
> simpler and a lot more straight-forward.

Yes having to set all the GIT_CONFIG_* variables can be rather confusing

Best Wishes

Phillip