git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Glen Choo" <chooglen@google.com>
Subject: Re: [PATCH v6 5/5] setup.c: create `discovery.bare`
Date: Thu, 30 Jun 2022 21:30:54 -0400	[thread overview]
Message-ID: <Yr5OTq7s2qxxqsiY@nand.local> (raw)
In-Reply-To: <a1323d963f917df661a8701c305d84e781a8f550.1656612839.git.gitgitgadget@gmail.com>

On Thu, Jun 30, 2022 at 06:13:59PM +0000, Glen Choo via GitGitGadget wrote:
> If we want to protect users from such attacks by default, neither value
> will suffice - "always" provides no protection, but "never" is
> impractical for bare repository users. A more usable default would be to
> allow only non-embedded bare repositories ([2] contains one such
> proposal), but detecting if a repository is embedded is potentially
> non-trivial, so this work is not implemented in this series.

I think that everything you said in your patch message makes sense, but
I appreciate this paragraph in particular. The historical record is
definitely important and worth preserving here, and I hope that it'll be
helpful to future readers who may wonder why the default wasn't chosen
as "never".

> [1]: https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com
> [2]: https://lore.kernel.org/git/5b969c5e-e802-c447-ad25-6acc0b784582@github.com
>
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  Documentation/config.txt           |  2 ++
>  Documentation/config/discovery.txt | 23 ++++++++++++
>  setup.c                            | 57 +++++++++++++++++++++++++++++-
>  t/t0035-discovery-bare.sh          | 52 +++++++++++++++++++++++++++
>  4 files changed, 133 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/config/discovery.txt
>  create mode 100755 t/t0035-discovery-bare.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e284b042f22..9a5e1329772 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -409,6 +409,8 @@ include::config/diff.txt[]
>
>  include::config/difftool.txt[]
>
> +include::config/discovery.txt[]
> +
>  include::config/extensions.txt[]
>
>  include::config/fastimport.txt[]
> diff --git a/Documentation/config/discovery.txt b/Documentation/config/discovery.txt
> new file mode 100644
> index 00000000000..bbcf89bb0b5
> --- /dev/null
> +++ b/Documentation/config/discovery.txt
> @@ -0,0 +1,23 @@
> +discovery.bare::
> +	Specifies whether Git will work with a bare repository that it
> +	found during repository discovery. If the repository is

Is it clear from the context what "discovery" means here? It's probably
easier to describe what it isn't, which you kind of do in the next
sentence. But it may be clearer to say something like:

    Specifies whether Git will recognize bare repositories that aren't
    specified via the top-level `--git-dir` command-line option, or the
    `GIT_DIR` environment variable (see linkgit:git[1]).

> +This defaults to `always`, but this default may change in the future.

I think the default being subject to change is par for the course. It's
probably easy enough to just say "Defaults to 'always'" and leave it at
that.

> ++
> +If you do not use bare repositories in your workflow, then it may be
> +beneficial to set `discovery.bare` to `never` in your global config.
> +This will protect you from attacks that involve cloning a repository
> +that contains a bare repository and running a Git command within that
> +directory.

I think we still don't have a great answer for people who trust some
bare repositories (e.g., known-embedded repositories that are used for
testing) but not others. To be clear, I think that is a fine point to
concede with this direction.

But we should be clear about that limitation by stating that Git does
not support the "I trust some bare repositories to be safely
discoverable but not others".

> +static enum discovery_bare_allowed get_discovery_bare(void)
> +{
> +	enum discovery_bare_allowed result = DISCOVERY_BARE_ALWAYS;
> +	git_protected_config(discovery_bare_cb, &result);
> +	return result;
> +}
> +
> +static const char *discovery_bare_allowed_to_string(
> +	enum discovery_bare_allowed discovery_bare_allowed)
> +{
> +	switch (discovery_bare_allowed) {
> +	case DISCOVERY_BARE_NEVER:
> +		return "never";
> +	case DISCOVERY_BARE_ALWAYS:
> +		return "always";

> +	default:
> +		BUG("invalid discovery_bare_allowed %d",
> +		    discovery_bare_allowed);

Should we have a default case here since the case arms above are
exhaustive?

> +	}
> +	return NULL;
> +}
> +
>  enum discovery_result {
>  	GIT_DIR_NONE = 0,
>  	GIT_DIR_EXPLICIT,
> @@ -1151,7 +1195,8 @@ enum discovery_result {
>  	GIT_DIR_HIT_CEILING = -1,
>  	GIT_DIR_HIT_MOUNT_POINT = -2,
>  	GIT_DIR_INVALID_GITFILE = -3,
> -	GIT_DIR_INVALID_OWNERSHIP = -4
> +	GIT_DIR_INVALID_OWNERSHIP = -4,
> +	GIT_DIR_DISALLOWED_BARE = -5,
>  };
>
>  /*
> @@ -1248,6 +1293,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  		}
>
>  		if (is_git_directory(dir->buf)) {
> +			if (!get_discovery_bare())

Relying on NEVER being the zero value here seems fragile to me. Should
we check that `if (get_discovery_bare() == DISCOVERY_BARE_NEVER)` to be
more explicit here?

> +				return GIT_DIR_DISALLOWED_BARE;
>  			if (!ensure_valid_ownership(dir->buf))
>  				return GIT_DIR_INVALID_OWNERSHIP;
>  			strbuf_addstr(gitdir, ".");
> @@ -1394,6 +1441,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  		}
>  		*nongit_ok = 1;
>  		break;
> +	case GIT_DIR_DISALLOWED_BARE:
> +		if (!nongit_ok) {
> +			die(_("cannot use bare repository '%s' (discovery.bare is '%s')"),
> +			    dir.buf,
> +			    discovery_bare_allowed_to_string(get_discovery_bare()));
> +		}
> +		*nongit_ok = 1;
> +		break;
>  	case GIT_DIR_NONE:
>  		/*
>  		 * As a safeguard against setup_git_directory_gently_1 returning

Thanks,
Taylor

  reply	other threads:[~2022-07-01  1:31 UTC|newest]

Thread overview: 113+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-06 18:30 [PATCH] [RFC] setup.c: make bare repo discovery optional 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
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-07-06 17:44               ` Glen Choo
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-07-06 18:21               ` Glen Choo
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-07-06 22:42               ` Glen Choo
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 [this message]
2022-07-07 19:55               ` Glen Choo
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
2022-07-01 17:37             ` Glen Choo
2022-07-08 21:58               ` Ævar Arnfjörð Bjarmason
2022-07-12 20:47                 ` Glen Choo
2022-07-12 23:53                   ` Ævar Arnfjörð Bjarmason
2022-07-07 23:01           ` [PATCH v7 " Glen Choo via GitGitGadget
2022-07-07 23:01             ` [PATCH v7 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-07-07 23:43               ` Junio C Hamano
2022-07-08 17:01                 ` Glen Choo
2022-07-08 19:01                   ` Junio C Hamano
2022-07-08 21:38                     ` Glen Choo
2022-07-07 23:01             ` [PATCH v7 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-07-08  0:39               ` Junio C Hamano
2022-07-07 23:01             ` [PATCH v7 3/5] config: learn `git_protected_config()` Glen Choo via GitGitGadget
2022-07-07 23:01             ` [PATCH v7 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-07-07 23:01             ` [PATCH v7 5/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-07-08  1:07             ` [PATCH v7 0/5] config: introduce discovery.bare and protected config Junio C Hamano
2022-07-08 20:35               ` Glen Choo
2022-07-12 22:11                 ` Glen Choo
2022-07-14 21:27             ` [PATCH v8 0/5] config: introduce safe.bareRepository " Glen Choo via GitGitGadget
2022-07-14 21:27               ` [PATCH v8 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-07-14 21:27               ` [PATCH v8 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-07-14 21:27               ` [PATCH v8 3/5] config: learn `git_protected_config()` Glen Choo via GitGitGadget
2022-07-25 18:26                 ` SANITIZE=address failure on master (was: [PATCH v8 3/5] config: learn `git_protected_config()`) Ævar Arnfjörð Bjarmason
2022-07-25 20:15                   ` Glen Choo
2022-07-25 20:41                     ` Ævar Arnfjörð Bjarmason
2022-07-25 20:56                       ` Glen Choo
2022-07-14 21:28               ` [PATCH v8 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-07-14 21:28               ` [PATCH v8 5/5] setup.c: create `safe.bareRepository` Glen Choo via GitGitGadget

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=Yr5OTq7s2qxxqsiY@nand.local \
    --to=me@ttaylorr.com \
    --cc=avarab@gmail.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=jonathantanmy@google.com \
    --cc=sandals@crustytoothpaste.net \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).