Re: [PATCH v2 0/2] setup.c: make bare repo discovery optional

From: Glen Choo <chooglen@google.com>
Date: May 17, 2022, 18:56 UTC
Message-ID: <kl6llev0htsy.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <5b969c5e-e802-c447-ad25-6acc0b784582@github.com>

Thanks, I think this has advanced the conversation quite a bit.

Derrick Stolee <derrickstolee@github.com> writes:

Show 25 quoted lines

> On 5/13/2022 7:37 PM, Glen Choo via GitGitGadget wrote:
>> Thanks all for the comments on v1, I've expanded this series somewhat to
>> address them,...
>
> Please include a full cover letter with each version, so reviewers
> can respond to the full series goals.
>
> Your series here intends to start protecting against malicious
> embedded bare repositories by allowing users to opt-in to a more
> protected state. When the 'discovery.bare' option is set, then
> Git may die() on a bare repository that is discovered based on
> the current working directory (these protections are ignored if
> the user specifies the directory directly through --git-dir or
> $GIT_DIR).
>
> The 'discovery.bare' option has these values at the end of your
> series:
>
> * 'always' (default) allows all bare repos, matching the current
>   behavior of Git.
>
> * 'never' avoids operating in bare repositories altogether.
>
> * 'cwd' operates in a bare repository only if the current directory
>   is exactly the root of the bare repository.

My mistake, I should have prepared this summary myself. Thanks again.

> It is important that we keep 'always' as the default at first,
> because we do not want to introduce a breaking change without
> warning (at least for an issue like this that has been around
> for a long time).

Yes.

Show 6 quoted lines

> The 'never' option is a good one for very security-conscious
> users who really want to avoid problems. I don't anticipate that
> users who know about this option and set it themselves are the
> type that would fall for the social engineering required to
> attack using this vector, but I can imagine an IT department
> installing the value in system config across a fleet of machines.

Yes. Setting the 'never' option in a system config is the use case that motivated this.

> I find the 'cwd' option to not be valuable. It unblocks most
> existing users, but also almost completely removes the protection
> that the option was supposed to provide.

Ok, I agree that it provides next-to-no protection. I'll drop it in this series; it's easy enough to reimplement if users really want it anyway.

Show 35 quoted lines

> This leads to what I think would be a valuable replacement for
> the 'cwd' option:
>
> * 'no-embedded' allows non-embedded bare repositories. An
>   _embedded bare repository_ is a bare repository whose parent
>   directory is contained in the worktree of a non-bare Git
>   repository. When in this mode, embedded bare repositories are
>   not allowed unless the parent non-bare Git repository has a
>   'safe.embedded' config value storing the path to the current
>   embedded bare repository.
>
> That was certainly difficult to write, but here it is as
> pseudo-code to hopefully remove some doubt as to how this might
> work:
>
>   if repo is bare:
>     if value == "always":
>        return ALLOWED
>     if value == "never":
>        return FORBIDDEN;
>
>     path = get_parent_repo()
>
>     if !path:
>        return ALLOWED
>     
>     if config_file_has_value("{path}/.git/config", "safe.embedded", repo):
>        return ALLOWED
>
>     return FORBIDDEN
>
> With this kind of option, we can protect users from these
> social engineering attacks while providing an opt-in protection
> for scenarios where embedded bare repos are currently being used
> (while also not breaking anyone using non-embedded bare repos).

[...]

> This 'no-embedded' option is something that I could see as a
> potential new default, after it has proven itself in a released
> version of Git.

I agree, this sounds like a good default that should work for most users.

That said, I don't think I will implement it, and even if I do, it won't be in this series. I have serious doubts that I'd be able to deliver it in a reasonable amount of time (I tried preparing patches to this effect and failed [1]), and 'never' is sufficient for $DAYJOB's current needs.

I would be very happy to see this come to fruition though. I have no objections to anyone preparing patches for this, and I'll gladly review those if that's helpful.

[1] The specific trouble I had was figuring out whether or not the
 'parent' repo was tracking the bare repo, since an untracked bare repo
 in the working tree isn't (in some sense) really "embedded" and it
 can't have come from a remote.

 But maybe the tracking check is unnecessary. We would break a few more
 users without it, but 'safe.embedded' is an easy enough way for a user
 to unbreak themselves.

Show 6 quoted lines

> I mentioned some other concerns in your PATCH 1 about how we
> are now adding the third use of read_very_early_config() and that
> we should probably refactor that before adding the third option,
> in order to avoid additional performance costs as well as it
> being difficult to audit which config options are only checked
> from these "protected" config files.

Makes sense. I'll ask about specifics on that subthread.

>
> Thanks,
> -Stolee