From: Derrick Stolee <derrickstolee@github.com>
To: 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>,
Glen Choo <chooglen@google.com>
Subject: Re: [PATCH v2 1/2] setup.c: make bare repo discovery optional
Date: Mon, 16 May 2022 14:46:55 -0400 [thread overview]
Message-ID: <e5139922-9b74-ebfa-756c-629918e0456b@github.com> (raw)
In-Reply-To: <22b10bf9da8ccf4ae4da634aadfdaff5ee7a3508.1652485058.git.gitgitgadget@gmail.com>
On 5/13/2022 7:37 PM, Glen Choo via GitGitGadget wrote:
> From: Glen Choo <chooglen@google.com>
>
> Add a config variable, `discovery.bare`, that tells Git whether or not
> it should work with the bare repository it has discovered i.e. Git will
> die() if it discovers a bare repository, but it is not allowed by
> `discovery.bare`. This only affects repository discovery, thus it has no
> effect if discovery was not done (e.g. `--git-dir` was passed).
> This config is an enum of:
>
> - ["always"|(unset)]: always recognize bare repositories (like Git does
> today)
> - "never": never recognize bare repositories
>
> More values are expected to be added later, and the default is expected
> to change (i.e. to something other than "always").
I think it is fine to include the "never" option for users to opt-in to
this super-protected state, but I want to make it very clear that we
should never move to it as a new default. This phrasing of 'something
other than "always"' is key, but it might be good to point out that
"never" is very unlikely to be that default.
> WIP setup.c: make discovery.bare die on failure
>
> Signed-off-by: Glen Choo <chooglen@google.com>
Accidental concatenation of squashed commit?
> diff --git a/Documentation/config/discovery.txt b/Documentation/config/discovery.txt
> new file mode 100644
> index 00000000000..761cabe6e70
> --- /dev/null
> +++ b/Documentation/config/discovery.txt
> @@ -0,0 +1,24 @@
> +discovery.bare::
> + Specifies what kinds of directories Git can recognize as a bare
> + repository when looking for the repository (aka repository
> + discovery). This has no effect if repository discovery is not
> + performed e.g. the path to the repository is set via `--git-dir`
> + (see linkgit:git[1]).
Avoid "e.g." here.
This has no effect if the repository is specified directly via
the --git-dir command-line option or the GIT_DIR environment
variable.
> +This config setting is only respected when specified in a system or global
> +config, not when it is specified in a repository config or via the command
> +line option `-c discovery.bare=<value>`.
We are sprinkling config options that have these same restrictions throughout
the config documentation. It might be time to define a term like "protected
config" at the top of git-config.txt and then refer to that from these other
locations.
> +The currently supported values are `always` (Git always recognizes bare
> +repositories) and `never` (Git never recognizes bare repositories).
This sentence structure is likely to change in the future, and as it stands
will become complicated. A bulleted list will have easier edits in the future.
> +This defaults to `always`, but this default is likely to change.
For now, I would say "but this default may change in the future." instead.
> +If your workflow does not rely on bare repositories, it is recommended that
> +you set this value to `never`. This makes repository discovery easier to
> +reason about and prevents certain types of security and non-security
> +problems, such as:
> +
(You might need a "+" here.)
> +* `git clone`-ing a repository containing a malicious bare repository
> + inside it.
> +* Git recognizing a directory that isn't meant to be a bare repository,
> + but happens to look like one.
I think these last bits recommending the 'never' option are a bit
distracting. It doesn't make repository discovery "easier to reason
about" because we still discover the bare repo and die() instead of
skipping it and looking higher for a non-bare repository in the
parent directories. The case of an "accidentally-recognized bare
repo" is so unlikely it is probably not worth mention in these docs.
Instead, I think something like this might be better:
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.
> +static int check_bare_repo_allowed(void)
> +{
> + if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
> + read_very_early_config(discovery_bare_cb, NULL);
This will add the third place where we use read_very_early_config(),
adding to the existing calls in tr2_sysenv_load() and
ensure_valid_ownership(). If I understand it correctly, that means
that every Git execution in a bare repository will now parse the
system and global config three times.
This doesn't count the check for uploadpack.packobjectshook in
upload-pack.c that uses current_config_scope() to restrict its
value to the system and global config.
We are probably at the point where we need to instead create a
configset that stores this "protected config" and allow us to
lookup config keys directly from that configset instead of
iterating through these config files repeatedly.
> + /* We didn't find a value; use the default. */
> + if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN)
> + discovery_bare_config = DISCOVERY_BARE_ALWAYS;
This could also be done in advance of the config parsing
by setting discovery_bare_config = DISCOVERY_BARE_ALWAYS before
calling read_very_early_config(). Avoids an if and a comment
here, which might be nice.
> + }
> + switch (discovery_bare_config) {
> + case DISCOVERY_BARE_NEVER:
> + return 0;
> + case DISCOVERY_BARE_ALWAYS:
> + return 1;
> + default:
> + BUG("invalid discovery_bare_config %d", discovery_bare_config);
> + }
You return -1 in discovery_bare_cb when the key matches, but
the value is not understood. Should we check the return value
of read_very_early_config(), too?
> +static const char *discovery_bare_config_to_string(void)
> +{
> + switch (discovery_bare_config) {
> + case DISCOVERY_BARE_NEVER:
> + return "never";
> + case DISCOVERY_BARE_ALWAYS:
> + return "always";
> + default:
> + BUG("invalid discovery_bare_config %d", discovery_bare_config);
In general, I'm not sure these BUG() statements are helpful,
but they aren't hurting anything. I wonder if it would be
better to use DISCOVERY_BARE_UNKNOWN instead of default,
because then the compiler should notice that the switch needs
updating when a new enum mode is added.
> @@ -1142,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
I think that you can add a comma at the end of this enum to avoid the
changed line the next time the enum needs to be expanded.
> };
>
> /*
> @@ -1239,6 +1293,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> }
>
> if (is_git_directory(dir->buf)) {
> + if (!check_bare_repo_allowed())
> + return GIT_DIR_DISALLOWED_BARE;
Won't this fail if someone runs a Git command inside of a .git/
directory for a non-bare repository? I just want to be sure that
we hit this error instead:
fatal: this operation must be run in a work tree
I see that this error is tested in t0008-ignores.sh, but that's
with the default "always" value. It would be good to explicitly
check that this is the right error when using the "never" config.
Thanks,
-Stolee
next prev parent reply other threads:[~2022-05-16 18:47 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 [this message]
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
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=e5139922-9b74-ebfa-756c-629918e0456b@github.com \
--to=derrickstolee@github.com \
--cc=chooglen@google.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 \
/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).