git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Glen Choo <chooglen@google.com>
To: Derrick Stolee <derrickstolee@github.com>,
	Glen Choo via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Taylor Blau <me@ttaylorr.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Junio C Hamano <gitster@pobox.com>,
	Emily Shaffer <emilyshaffer@google.com>
Subject: Re: [PATCH v2 0/2] setup.c: make bare repo discovery optional
Date: Tue, 17 May 2022 11:56:29 -0700	[thread overview]
Message-ID: <kl6llev0htsy.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
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:

> 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.

> 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.

> 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.

> 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

  parent reply	other threads:[~2022-05-17 18:56 UTC|newest]

Thread overview: 81+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-06 18:30 [PATCH] [RFC] " Glen Choo via GitGitGadget
2022-05-06 20:33 ` Junio C Hamano
2022-05-09 21:42 ` Taylor Blau
2022-05-09 22:54   ` Junio C Hamano
2022-05-09 23:57     ` Taylor Blau
2022-05-10  0:23       ` Junio C Hamano
2022-05-10 22:00   ` Glen Choo
2022-05-13 23:37 ` [PATCH v2 0/2] " Glen Choo via GitGitGadget
2022-05-13 23:37   ` [PATCH v2 1/2] " Glen Choo via GitGitGadget
2022-05-16 18:12     ` Glen Choo
2022-05-16 18:46     ` Derrick Stolee
2022-05-16 22:25       ` Taylor Blau
2022-05-17 20:24       ` Glen Choo
2022-05-17 21:51         ` Glen Choo
2022-05-13 23:37   ` [PATCH v2 2/2] setup.c: learn discovery.bareRepository=cwd Glen Choo via GitGitGadget
2022-05-16 18:49     ` Derrick Stolee
2022-05-16 16:40   ` [PATCH v2 0/2] setup.c: make bare repo discovery optional Junio C Hamano
2022-05-16 18:36     ` Glen Choo
2022-05-16 19:16       ` Junio C Hamano
2022-05-16 20:27         ` Glen Choo
2022-05-16 22:16           ` Junio C Hamano
2022-05-16 16:43   ` Junio C Hamano
2022-05-16 19:07   ` Derrick Stolee
2022-05-16 22:43     ` Taylor Blau
2022-05-16 23:19     ` Junio C Hamano
2022-05-17 18:56     ` Glen Choo [this message]
2022-05-27 21:09   ` [PATCH v3 0/5] config: introduce discovery.bare and protected config Glen Choo via GitGitGadget
2022-05-27 21:09     ` [PATCH v3 1/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-05-27 23:29       ` Junio C Hamano
2022-06-02 12:42         ` Derrick Stolee
2022-06-02 16:53           ` Junio C Hamano
2022-06-02 17:39             ` Glen Choo
2022-06-03 15:57         ` Glen Choo
2022-05-27 21:09     ` [PATCH v3 2/5] config: read protected config with `git_protected_config()` Glen Choo via GitGitGadget
2022-05-28  0:28       ` Junio C Hamano
2022-05-31 17:43         ` Glen Choo
2022-06-01 15:58           ` Junio C Hamano
2022-06-02 12:56       ` Derrick Stolee
2022-05-27 21:09     ` [PATCH v3 3/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-05-28  0:59       ` Junio C Hamano
2022-06-02 13:11       ` Derrick Stolee
2022-05-27 21:09     ` [PATCH v3 4/5] config: include "-c" in protected config Glen Choo via GitGitGadget
2022-06-02 13:15       ` Derrick Stolee
2022-05-27 21:09     ` [PATCH v3 5/5] upload-pack: make uploadpack.packObjectsHook protected Glen Choo via GitGitGadget
2022-06-02 13:18       ` Derrick Stolee
2022-06-07 20:57     ` [PATCH v4 0/5] config: introduce discovery.bare and protected config Glen Choo via GitGitGadget
2022-06-07 20:57       ` [PATCH v4 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-06-07 20:57       ` [PATCH v4 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-06-22 21:58         ` Jonathan Tan
2022-06-23 18:21           ` Glen Choo
2022-06-07 20:57       ` [PATCH v4 3/5] config: read protected config with `git_protected_config()` Glen Choo via GitGitGadget
2022-06-07 22:49         ` Junio C Hamano
2022-06-08  0:22           ` Glen Choo
2022-06-07 20:57       ` [PATCH v4 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-06-07 20:57       ` [PATCH v4 5/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-06-07 21:37         ` Glen Choo
2022-06-22 22:03       ` [PATCH v4 0/5] config: introduce discovery.bare and protected config Jonathan Tan
2022-06-23 17:13         ` Glen Choo
2022-06-23 18:32           ` Junio C Hamano
2022-06-27 17:34             ` Glen Choo
2022-06-27 18:19       ` Glen Choo
2022-06-27 18:36       ` [PATCH v5 " Glen Choo via GitGitGadget
2022-06-27 18:36         ` [PATCH v5 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-06-27 18:36         ` [PATCH v5 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-06-27 18:36         ` [PATCH v5 3/5] config: learn `git_protected_config()` Glen Choo via GitGitGadget
2022-06-27 18:36         ` [PATCH v5 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-06-27 18:36         ` [PATCH v5 5/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-06-30 13:20           ` Ævar Arnfjörð Bjarmason
2022-06-30 17:28             ` Glen Choo
2022-06-30 18:13         ` [PATCH v6 0/5] config: introduce discovery.bare and protected config Glen Choo via GitGitGadget
2022-06-30 18:13           ` [PATCH v6 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-06-30 22:32             ` Taylor Blau
2022-06-30 18:13           ` [PATCH v6 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-06-30 23:49             ` Taylor Blau
2022-06-30 18:13           ` [PATCH v6 3/5] config: learn `git_protected_config()` Glen Choo via GitGitGadget
2022-07-01  1:22             ` Taylor Blau
2022-06-30 18:13           ` [PATCH v6 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-06-30 18:13           ` [PATCH v6 5/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-07-01  1:30             ` Taylor Blau
2022-06-30 22:13           ` [PATCH v6 0/5] config: introduce discovery.bare and protected config Taylor Blau
2022-06-30 23:07           ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=kl6llev0htsy.fsf@chooglen-macbookpro.roam.corp.google.com \
    --to=chooglen@google.com \
    --cc=derrickstolee@github.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [PATCH v2 0/2] setup.c: make bare repo discovery optional' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).