git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Junio C Hamano <gitster@pobox.com>,
	Emily Shaffer <emilyshaffer@google.com>,
	Glen Choo <chooglen@google.com>
Subject: Re: [PATCH v2 0/2] setup.c: make bare repo discovery optional
Date: Mon, 16 May 2022 18:43:55 -0400	[thread overview]
Message-ID: <YoLTqxYU7VO91Dam@nand.local> (raw)
In-Reply-To: <5b969c5e-e802-c447-ad25-6acc0b784582@github.com>

On Mon, May 16, 2022 at 03:07:35PM -0400, Derrick Stolee wrote:
> 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. [...]

Thanks for the summary, which I think will be especially helpful to
others looking at this series for the very first time.

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

When I first read this, I disagreed, since presumably that same crowd
has legitimate bare repositories that they want to continue being able
to operate in without having to pass `--git-dir` or `$GIT_DIR` in.

In fact...

> I also think that this protection is too rigid: it restricts
> _all_ bare repositories, not just embedded ones. There is no check
> to see if the parent directory of the bare repository is inside a
> non-bare repository.

...this resonates quite a bit more with me. "never" isn't a good option
unless you aren't a user of bare repositories _and_ don't have any
embedded bare repositories (either at all, or any ones that you trust).

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

This is indeed very similar to a proposal I had made upthread (which you
note lower down in this email). One thing that's nice is that we only
have to traverse up to the parent repo when in the "no-embedded" mode.
That may be slow (since it's unbounded all the way up to the filesystem
root or a ceiling directory, whichever we encounter first), but I think
it's unavoidable if you need to distinguish between embedded and
non-embedded bare repositories.

>     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).
>
> I think Taylor was mentioning something like this in his previous
> replies, perhaps even to the previous thread on this topic.

Yep, see: https://lore.kernel.org/git/Ylobp7sntKeWTLDX@nand.local/.a

> 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 would be totally happy to see "no-embedded" become the default. It
might be nice to issue a warning when the top-level config is unset,
to give users a heads up about cases that may be broken, perhaps like:

    if repo is bare:
      switch (value) {
      case "always":
        return ALLOWED;
      case "never":
        return FORBIDDEN;
      case "no-embedded": # fallthrough
      case "":
        path = get_parent_repo()
        if !path
          return ALLOWED;

        if config_file_has_value("{path}/.git/config", "safe.embedded", repo)
          return ALLOWED;

        if value == "no-embedded":
          return FORBIDDEN;

        # otherwise, we're in an embedded bare repository with an unset
        # discovery.bare config.
        #
        # warn that this will break in the future...
        warning(_("%s is embedded within %s"), the_repository.path, path);
        advise(_("to allow discovery for this embedded repo, either run"));
        advise(_(""));
        advise(_("  $ git config --global discovery.bare always, or"));
        advise(_("  $ git -C '%s' config --local safe.embedded '%s'"),
               path, relpath(path, the_repository.path));

        # ...but allow the invocation for now until the default is
        # changed.
        return ALLOWED;
      default:
        die(_("unrecognized value of discovery.bare: '%s'"), value);
      }

...where relpath is similar to Go's path/filepath.Rel function.

With an appropriate deprecation period, I think we could even get away
from the "continue executing, but don't read config+hooks", which in
retrospect is more error-prone and difficult to reason about than I
initially had given it credit for.

Thanks,
Taylor

  reply	other threads:[~2022-05-16 22:44 UTC|newest]

Thread overview: 79+ 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 [this message]
2022-05-16 23:19     ` Junio C Hamano
2022-05-17 18:56     ` Glen Choo
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-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-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=YoLTqxYU7VO91Dam@nand.local \
    --to=me@ttaylorr.com \
    --cc=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=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).