| Message ID | e98be8e7f703fc741e06d9208545abc8c24d1a4a.1682962110.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e35f202b4503256db148ad61487fe13aa75960f2 |
| Headers | show |
| Series | [v3] setup: trace bare repository setups | expand |
Josh Steadmon <steadmon@google.com> writes: > From: Glen Choo <chooglen@google.com> > > safe.bareRepository=explicit is a safer default mode of operation, since > it guards against the embedded bare repository attack [1]. Most end > users don't use bare repositories directly, so they should be able to > set safe.bareRepository=explicit, with the expectation that they can > reenable bare repositories by specifying GIT_DIR or --git-dir. > > However, the user might use a tool that invokes Git on bare repositories > without setting GIT_DIR (e.g. "go mod" will clone bare repositories > [2]), so even if a user wanted to use safe.bareRepository=explicit, it > wouldn't be feasible until their tools learned to set GIT_DIR. > > To make this transition easier, add a trace message to note when we > attempt to set up a bare repository without setting GIT_DIR. This allows > users and tool developers to audit which of their tools are problematic > and report/fix the issue. When they are sufficiently confident, they > would switch over to "safe.bareRepository=explicit". > > Note that this uses trace2_data_string(), which isn't supported by the > "normal" GIT_TRACE2 target, only _EVENT or _PERF. > > [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/ > [2] https://go.dev/ref/mod > > Signed-off-by: Glen Choo <chooglen@google.com> > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > I'm sending a lightly-adapted version of Glen's tracing patch because > Glen will be on vacation next week and we'd like to get this upstream > ASAP. > > Changes in V3: added a test_unconfig test case for safe.bareRepository > Changes in V2: cleaned up test-style issues. Thanks. We saw no interest on the list in reviewing this patch further, it seems, but I didn't see anything glaringly wrong, see no reason not to merge it, and this should help noticing potential issues by $corp folks, I would presume, so let's merge it as-is.
On Fri, May 05, 2023 at 03:30:38PM -0700, Junio C Hamano wrote: > Thanks. We saw no interest on the list in reviewing this patch > further, it seems, but I didn't see anything glaringly wrong, see > no reason not to merge it, and this should help noticing potential > issues by $corp folks, I would presume, so let's merge it as-is. I took a look through this thread and would be fine to see this one picked up, though I did have a couple of questions: - Is the plan to eventually disable $GIT_DIR discovery in bare repositories by default in a future version? I am still uncertain of the assumption that most end-users don't interact with bare repositories directly. Certainly forges touch bare repositories without always setting $GIT_DIR in their environment. But I would imagine that other tools indirectly touch bare repositories on behalf of the user. You mentioned "go" as one such tool that doesn't set $GIT_DIR, I imagine there are many more. - If it is the plan to disable $GIT_DIR discovery in bare repositories in the future, I'm not sure how visible the extra trace line would be. Perhaps that is desirable, since having an advise() call on every Git invocation in a bare repository would be noisy. Thanks, Taylor
On 2023.05.08 18:31, Taylor Blau wrote: > On Fri, May 05, 2023 at 03:30:38PM -0700, Junio C Hamano wrote: > > Thanks. We saw no interest on the list in reviewing this patch > > further, it seems, but I didn't see anything glaringly wrong, see > > no reason not to merge it, and this should help noticing potential > > issues by $corp folks, I would presume, so let's merge it as-is. > > I took a look through this thread and would be fine to see this one > picked up, though I did have a couple of questions: > > - Is the plan to eventually disable $GIT_DIR discovery in bare > repositories by default in a future version? I am still uncertain > of the assumption that most end-users don't interact with bare > repositories directly. I think at some point in the future, we'd like for `safe.bareRepository=explicit` to be the default. To get to a state where we're comfortable making that change, we plan in the near-ish future to flip the default when `feature.experimental` is enabled. > Certainly forges touch bare repositories without always setting > $GIT_DIR in their environment. But I would imagine that other tools > indirectly touch bare repositories on behalf of the user. You > mentioned "go" as one such tool that doesn't set $GIT_DIR, I imagine > there are many more. > > - If it is the plan to disable $GIT_DIR discovery in bare repositories > in the future, I'm not sure how visible the extra trace line would > be. Perhaps that is desirable, since having an advise() call on > every Git invocation in a bare repository would be noisy. Yeah, this is mainly to help us plan for our internal rollout of `safe.bareRepository=explicit` at $DAYJOB, but we assume it would be helpful for others who might also be considering this, or for developers of affected tooling when they receive a bug report that bareRepository breaks their users. We also want to add some more detailed tracing in general around repo/worktree initialization, but that part is not so urgent.
diff --git a/setup.c b/setup.c index 59abc16ba6..458582207e 100644 --- a/setup.c +++ b/setup.c @@ -1352,6 +1352,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, } if (is_git_directory(dir->buf)) { + trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf); if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT) return GIT_DIR_DISALLOWED_BARE; if (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh index 11c15a48aa..038b8b788d 100755 --- a/t/t0035-safe-bare-repository.sh +++ b/t/t0035-safe-bare-repository.sh @@ -7,13 +7,26 @@ TEST_PASSES_SANITIZE_LEAK=true pwd="$(pwd)" -expect_accepted () { - git "$@" rev-parse --git-dir +expect_accepted_implicit () { + test_when_finished 'rm "$pwd/trace.perf"' && + GIT_TRACE2_PERF="$pwd/trace.perf" git "$@" rev-parse --git-dir && + # Note: we're intentionally only checking that the bare repo has a + # directory *prefix* of $pwd + grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf" +} + +expect_accepted_explicit () { + test_when_finished 'rm "$pwd/trace.perf"' && + GIT_DIR="$1" GIT_TRACE2_PERF="$pwd/trace.perf" git rev-parse --git-dir && + ! grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf" } expect_rejected () { - test_must_fail git "$@" rev-parse --git-dir 2>err && - grep -F "cannot use bare repository" err + test_when_finished 'rm "$pwd/trace.perf"' && + test_env GIT_TRACE2_PERF="$pwd/trace.perf" \ + test_must_fail git "$@" rev-parse --git-dir 2>err && + grep -F "cannot use bare repository" err && + grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf" } test_expect_success 'setup bare repo in worktree' ' @@ -22,12 +35,13 @@ test_expect_success 'setup bare repo in worktree' ' ' test_expect_success 'safe.bareRepository unset' ' - expect_accepted -C outer-repo/bare-repo + test_unconfig --global safe.bareRepository && + expect_accepted_implicit -C outer-repo/bare-repo ' test_expect_success 'safe.bareRepository=all' ' test_config_global safe.bareRepository all && - expect_accepted -C outer-repo/bare-repo + expect_accepted_implicit -C outer-repo/bare-repo ' test_expect_success 'safe.bareRepository=explicit' ' @@ -47,7 +61,7 @@ test_expect_success 'safe.bareRepository in the repository' ' test_expect_success 'safe.bareRepository on the command line' ' test_config_global safe.bareRepository explicit && - expect_accepted -C outer-repo/bare-repo \ + expect_accepted_implicit -C outer-repo/bare-repo \ -c safe.bareRepository=all ' @@ -60,4 +74,8 @@ test_expect_success 'safe.bareRepository in included file' ' expect_rejected -C outer-repo/bare-repo ' +test_expect_success 'no trace when GIT_DIR is explicitly provided' ' + expect_accepted_explicit "$pwd/outer-repo/bare-repo" +' + test_done