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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > [2]: https://lore.kernel.org/git/5b969c5e-e802-c447-ad25-6acc0b784582@xxxxxxxxxx > > Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx> > --- > 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
