threads / patch / 59665

patchsetup: trace bare repository setups

Subject: [PATCH] setup: trace bare repository setups

## tl;dr

16 messages between Apr 27, 2023 and May 10, 2023. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Josh Steadmon· Apr 27, 2023, 22:32 UTC · lore
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>
Change-Id: I8e8b5e70ce8c6c81ec4716187c27c44da38b35db
---
 setup.c                         |  1 +
 t/t0035-safe-bare-repository.sh | 29 ++++++++++++++++++++++-------
 2 files changed, 23 insertions(+), 7 deletions(-)
Show changes to 2 files +23 −7

setup.c, t/t0035-safe-bare-repository.sh

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..a1f3b5a4d6 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -7,13 +7,24 @@ 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 &&
+	grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
+}
+
+expect_accepted_explicit () {
+	test_when_finished "rm \"$pwd/trace.perf\"" &&
+	GIT_DIR="$@" 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 +33,12 @@ test_expect_success 'setup bare repo in worktree' '
 '
 
 test_expect_success 'safe.bareRepository unset' '
-	expect_accepted -C outer-repo/bare-repo
+	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 +58,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 +71,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

base-commit: 2807bd2c10606e590886543afe4e4f208dddd489
-- 
2.40.1.495.gc816e09b53d-goog
Junio C Hamano· Apr 27, 2023, 22:54 UTC · re: Josh Steadmon · lore

Re: [PATCH] setup: trace bare repository setups

Josh Steadmon <steadmon@google.com> writes:
Show 12 quoted lines
> 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))
That is kind of obvious.
Show 12 quoted lines
> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> index 11c15a48aa..a1f3b5a4d6 100755
> --- a/t/t0035-safe-bare-repository.sh
> +++ b/t/t0035-safe-bare-repository.sh
> @@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true
>  
>  pwd="$(pwd)"
>  
> -expect_accepted () {
> -	git "$@" rev-parse --git-dir
> +expect_accepted_implicit () {
> +	test_when_finished "rm \"$pwd/trace.perf\"" &&
Why not
	test_when_finished 'rm "$pwd/trace.perf"' &&
instead?  

In your version, $pwd is expanded before test_when_finished sees it, so you'd have to worry about things like backslashes and double quotes in it. You can of course quote the '$' like so

	test_when_finished "rm \"\$pwd/trace.perf\"" &&

to work it around, but it is equivalent to enclosing everything inside a pair of single quotes. Either way your $pwd will be interpolated when "eval" sees the $test_cleanup script.

And it would be much easier to read without backslash and backslashed double quotes.

> +	GIT_TRACE2_PERF="$pwd/trace.perf" git "$@" rev-parse --git-dir &&
> +	grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
> +}
We ensure that we positively have such a trace in the output.  Good.
Show 5 quoted lines
> +expect_accepted_explicit () {
> +	test_when_finished "rm \"$pwd/trace.perf\"" &&
> +	GIT_DIR="$@" GIT_TRACE2_PERF="$pwd/trace.perf" git rev-parse --git-dir &&
> +	! grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
>  }

When not asking for the magic behaviour of "$@" and instead doing a "squash everything into a single string, using the first letter in $IFS as the separator in between", please write "$*" instead.

    GIT_DIR="$*" GIT_TRACE2_PERF="..." git rev-parse --git-dir

But in this case, I do not think you are ever planning to send a directory name split into two or more, to be concatenated with SP, so writing it like

    GIT_DIR="$1" GIT_TRACE2_PERF="..." git rev-parse --git-dir
would be much less error prone and easier to follow, I think.
Show 7 quoted lines
> @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '
>  '
>  
>  test_expect_success 'safe.bareRepository unset' '
> -	expect_accepted -C outer-repo/bare-repo
> +	expect_accepted_implicit -C outer-repo/bare-repo
>  '

Perhaps futureproof this test piece by explicitly unsetting the variable before starting the test? That way, this piece will not be broken even if earlier tests gets modified to set some value to safe.bareRepository in the future.

Show 25 quoted lines
>  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 +58,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 +71,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
All the expectations look sensible.  Thanks for a pleasant read.
Josh Steadmon· Apr 28, 2023, 16:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] setup: trace bare repository setups

On 2023.04.27 15:54, Junio C Hamano wrote:
Show 115 quoted lines
> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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))
> 
> That is kind of obvious.
> 
> > diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> > index 11c15a48aa..a1f3b5a4d6 100755
> > --- a/t/t0035-safe-bare-repository.sh
> > +++ b/t/t0035-safe-bare-repository.sh
> > @@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true
> >  
> >  pwd="$(pwd)"
> >  
> > -expect_accepted () {
> > -	git "$@" rev-parse --git-dir
> > +expect_accepted_implicit () {
> > +	test_when_finished "rm \"$pwd/trace.perf\"" &&
> 
> Why not
> 
> 	test_when_finished 'rm "$pwd/trace.perf"' &&
> 
> instead?  
> 
> In your version, $pwd is expanded before test_when_finished sees it,
> so you'd have to worry about things like backslashes and double quotes
> in it.  You can of course quote the '$' like so
> 
> 	test_when_finished "rm \"\$pwd/trace.perf\"" &&
> 
> to work it around, but it is equivalent to enclosing everything
> inside a pair of single quotes.  Either way your $pwd will be
> interpolated when "eval" sees the $test_cleanup script.
> 
> And it would be much easier to read without backslash and
> backslashed double quotes.
> 
> > +	GIT_TRACE2_PERF="$pwd/trace.perf" git "$@" rev-parse --git-dir &&
> > +	grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
> > +}
> 
> We ensure that we positively have such a trace in the output.  Good.
> 
> > +expect_accepted_explicit () {
> > +	test_when_finished "rm \"$pwd/trace.perf\"" &&
> > +	GIT_DIR="$@" GIT_TRACE2_PERF="$pwd/trace.perf" git rev-parse --git-dir &&
> > +	! grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
> >  }
> 
> When not asking for the magic behaviour of "$@" and instead doing a
> "squash everything into a single string, using the first letter in
> $IFS as the separator in between", please write "$*" instead.
> 
>     GIT_DIR="$*" GIT_TRACE2_PERF="..." git rev-parse --git-dir
> 
> But in this case, I do not think you are ever planning to send a
> directory name split into two or more, to be concatenated with SP,
> so writing it like
> 
>     GIT_DIR="$1" GIT_TRACE2_PERF="..." git rev-parse --git-dir
> 
> would be much less error prone and easier to follow, I think.
> 
> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '
> >  '
> >  
> >  test_expect_success 'safe.bareRepository unset' '
> > -	expect_accepted -C outer-repo/bare-repo
> > +	expect_accepted_implicit -C outer-repo/bare-repo
> >  '
> 
> Perhaps futureproof this test piece by explicitly unsetting the
> variable before starting the test?  That way, this piece will not be
> broken even if earlier tests gets modified to set some value to
> safe.bareRepository in the future.
> 
> >  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 +58,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 +71,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
> 
> All the expectations look sensible.  Thanks for a pleasant read.
Agreed with all the feedback points, will fix in V2. Thanks!
Josh Steadmon· Apr 28, 2023, 17:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] setup: trace bare repository setups

On 2023.04.27 15:54, Junio C Hamano wrote:
Show 14 quoted lines
> Josh Steadmon <steadmon@google.com> writes:
> 
> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '
> >  '
> >  
> >  test_expect_success 'safe.bareRepository unset' '
> > -	expect_accepted -C outer-repo/bare-repo
> > +	expect_accepted_implicit -C outer-repo/bare-repo
> >  '
> 
> Perhaps futureproof this test piece by explicitly unsetting the
> variable before starting the test?  That way, this piece will not be
> broken even if earlier tests gets modified to set some value to
> safe.bareRepository in the future.

Actually, explicitly setting the variable here is equivalent to the following test case, so I'll just remove this one.

Junio C Hamano· Apr 28, 2023, 20:26 UTC · re: Josh Steadmon · lore

Re: [PATCH] setup: trace bare repository setups

Josh Steadmon <steadmon@google.com> writes:
Show 18 quoted lines
> On 2023.04.27 15:54, Junio C Hamano wrote:
>> Josh Steadmon <steadmon@google.com> writes:
>> 
>> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '
>> >  '
>> >  
>> >  test_expect_success 'safe.bareRepository unset' '
>> > -	expect_accepted -C outer-repo/bare-repo
>> > +	expect_accepted_implicit -C outer-repo/bare-repo
>> >  '
>> 
>> Perhaps futureproof this test piece by explicitly unsetting the
>> variable before starting the test?  That way, this piece will not be
>> broken even if earlier tests gets modified to set some value to
>> safe.bareRepository in the future.
>
> Actually, explicitly setting the variable here is equivalent to the
> following test case, so I'll just remove this one.
I meant explicitly UNsetting, though?
Josh Steadmon· May 1, 2023, 17:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] setup: trace bare repository setups

On 2023.04.28 13:26, Junio C Hamano wrote:
Show 22 quoted lines
> Josh Steadmon <steadmon@google.com> writes:
> 
> > On 2023.04.27 15:54, Junio C Hamano wrote:
> >> Josh Steadmon <steadmon@google.com> writes:
> >> 
> >> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '
> >> >  '
> >> >  
> >> >  test_expect_success 'safe.bareRepository unset' '
> >> > -	expect_accepted -C outer-repo/bare-repo
> >> > +	expect_accepted_implicit -C outer-repo/bare-repo
> >> >  '
> >> 
> >> Perhaps futureproof this test piece by explicitly unsetting the
> >> variable before starting the test?  That way, this piece will not be
> >> broken even if earlier tests gets modified to set some value to
> >> safe.bareRepository in the future.
> >
> > Actually, explicitly setting the variable here is equivalent to the
> > following test case, so I'll just remove this one.
> 
> I meant explicitly UNsetting, though?

Ah, sorry for misunderstanding. I've restored the test along with a test_unconfig line for V3; however, doesn't this just turn into a "change detector" test whose only purpose will be to fail if/when we change the default value for this config option?

Glen Choo· May 8, 2023, 22:19 UTC · re: Josh Steadmon · lore

Re: [PATCH] setup: trace bare repository setups

Josh Steadmon <steadmon@google.com> writes:
> Ah, sorry for misunderstanding. I've restored the test along with a
> test_unconfig line for V3; however, doesn't this just turn into a "change
> detector" test whose only purpose will be to fail if/when we change the
> default value for this config option? 

Maybe a difference in opinion: "change detection" sounds like non-essential changes to program behavior can cause a test to fail, but default value handling seems quite essential to me. Not to mention that default value handling is easy to break, so IMO guarding against accidental regressions is pretty useful.

As for practical churn, I don't expect that we'll intentionally change this default often.

Glen Choo· Apr 27, 2023, 23:36 UTC · re: Josh Steadmon · lore

Re: [PATCH] setup: trace bare repository setups

Josh Steadmon <steadmon@google.com> writes:
> From: Glen Choo <chooglen@google.com>

I can confirm that I did write an initial version of this, which Josh cleaned up for the mailing list (thanks!). Most, if not all, of the mistakes are originally mine.

Show 5 quoted lines
> 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".

One alternative to this is to trace all of the repository setup process. E.g. if we traced the data points in t/t1510-repo-setup.sh, like GIT_DIR and whether the repository is bare, you could reverse-engineer whether we've hit the "set up a bare repository without GIT_DIR" case, but that's significantly more complicated. If the goal of this patch is to make it easy for users, tool developers and sysadmins to see if "safe.bareRepository=explicit" might be tripped, giving a single, meaningful event is much easier way to get there.

It would be nice to trace all of the repo setup eventually, anyway, and I don't think this change precludes that.

> Change-Id: I8e8b5e70ce8c6c81ec4716187c27c44da38b35db
Leftover from Gerrit, perhaps?

Unsurprisingly, I don't have comments on the diff, at least not anything that Junio hasn't already spotted.

Josh Steadmon· Apr 28, 2023, 16:48 UTC · re: Glen Choo · lore

Re: [PATCH] setup: trace bare repository setups

On 2023.04.27 16:36, Glen Choo wrote:
Show 32 quoted lines
> Josh Steadmon <steadmon@google.com> writes:
> 
> > From: Glen Choo <chooglen@google.com>
> 
> I can confirm that I did write an initial version of this, which Josh
> cleaned up for the mailing list (thanks!). Most, if not all, of the
> mistakes are originally mine.
> 
> > 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".
> 
> One alternative to this is to trace all of the repository setup process.
> E.g. if we traced the data points in t/t1510-repo-setup.sh, like GIT_DIR
> and whether the repository is bare, you could reverse-engineer whether
> we've hit the "set up a bare repository without GIT_DIR" case, but
> that's significantly more complicated. If the goal of this patch is to
> make it easy for users, tool developers and sysadmins to see if
> "safe.bareRepository=explicit" might be tripped, giving a single,
> meaningful event is much easier way to get there.
> 
> It would be nice to trace all of the repo setup eventually, anyway, and
> I don't think this change precludes that.
> 
> > Change-Id: I8e8b5e70ce8c6c81ec4716187c27c44da38b35db
> 
> Leftover from Gerrit, perhaps?
> 
> Unsurprisingly, I don't have comments on the diff, at least not anything
> that Junio hasn't already spotted.

Whoops, thanks for the catch. It would seem my send-email workflow broke somehow while I was away from work. I also had a comment explaining why I was sending this out for you, which somehow got dropped.

Josh Steadmon· Apr 28, 2023, 17:22 UTC · re: Josh Steadmon · lore

[PATCH v2] setup: trace bare repository setups

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.
Cleaned up some test-style issues since V1.
Range-diff against v1:
1:  cb72bca46c ! 1:  b548d96db7 setup: trace bare repository setups
    @@ t/t0035-safe-bare-repository.sh: TEST_PASSES_SANITIZE_LEAK=true
     -expect_accepted () {
     -	git "$@" rev-parse --git-dir
     +expect_accepted_implicit () {
    -+	test_when_finished "rm \"$pwd/trace.perf\"" &&
    ++	test_when_finished 'rm "$pwd/trace.perf"' &&
     +	GIT_TRACE2_PERF="$pwd/trace.perf" git "$@" rev-parse --git-dir &&
     +	grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
     +}
     +
     +expect_accepted_explicit () {
    -+	test_when_finished "rm \"$pwd/trace.perf\"" &&
    -+	GIT_DIR="$@" GIT_TRACE2_PERF="$pwd/trace.perf" git rev-parse --git-dir &&
    ++	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_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 &&
    @@ t/t0035-safe-bare-repository.sh: TEST_PASSES_SANITIZE_LEAK=true
      
      test_expect_success 'setup bare repo in worktree' '
     @@ t/t0035-safe-bare-repository.sh: test_expect_success 'setup bare repo in worktree' '
    + 	git init --bare outer-repo/bare-repo
      '
      
    - test_expect_success 'safe.bareRepository unset' '
    +-test_expect_success 'safe.bareRepository unset' '
     -	expect_accepted -C outer-repo/bare-repo
    -+	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
 setup.c                         |  1 +
 t/t0035-safe-bare-repository.sh | 31 +++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 10 deletions(-)
Show changes to 2 files +22 −10

setup.c, t/t0035-safe-bare-repository.sh

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..993f5bdc7d 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -7,13 +7,24 @@ 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 &&
+	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' '
@@ -21,13 +32,9 @@ test_expect_success 'setup bare repo in worktree' '
 	git init --bare outer-repo/bare-repo
 '
 
-test_expect_success 'safe.bareRepository unset' '
-	expect_accepted -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 +54,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 +67,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

base-commit: 2807bd2c10606e590886543afe4e4f208dddd489
-- 
2.40.1.495.gc816e09b53d-goog
Glen Choo· Apr 28, 2023, 18:37 UTC · re: Josh Steadmon · lore

Re: [PATCH v2] setup: trace bare repository setups

Josh Steadmon <steadmon@google.com> writes:
Show 21 quoted lines
> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> index 11c15a48aa..993f5bdc7d 100755
> --- a/t/t0035-safe-bare-repository.sh
> +++ b/t/t0035-safe-bare-repository.sh
> @@ -7,13 +7,24 @@ 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 &&
> +	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"
>  }
(Not new in v2) This wasn't obvious to me at first, but
  grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"

looks like it's asserting that we trace that the bare repository is $pwd, but it's actually only asserting that is starts with $pwd. If it might trip up other reasers, a comment here would be helpful.

Show 8 quoted lines
> @@ -21,13 +32,9 @@ test_expect_success 'setup bare repo in worktree' '
>  	git init --bare outer-repo/bare-repo
>  '
>  
> -test_expect_success 'safe.bareRepository unset' '
> -	expect_accepted -C outer-repo/bare-repo
> -'
> -

I found this surprising; I thought it was quite valuable to test the default behavior. I'd prefer to keep this test.

Perhaps there was a miscommunication? You mentioned:
  Actually, explicitly setting the variable here is equivalent to the
  following test case, so I'll just remove this one.

which is true, but I think Junio meant _un_setting the variable, something like:

  test_expect_success 'safe.bareRepository unset' '
+   test_unconfig --global safe.bareRepository &&
    expect_accepted -C outer-repo/bare-repo
  '
Josh Steadmon· May 1, 2023, 17:22 UTC · re: Glen Choo · lore

Re: [PATCH v2] setup: trace bare repository setups

On 2023.04.28 11:37, Glen Choo wrote:
Show 31 quoted lines
> Josh Steadmon <steadmon@google.com> writes:
> 
> > diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> > index 11c15a48aa..993f5bdc7d 100755
> > --- a/t/t0035-safe-bare-repository.sh
> > +++ b/t/t0035-safe-bare-repository.sh
> > @@ -7,13 +7,24 @@ 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 &&
> > +	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"
> >  }
> 
> (Not new in v2) This wasn't obvious to me at first, but
> 
>   grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
> 
> looks like it's asserting that we trace that the bare repository is
> $pwd, but it's actually only asserting that is starts with $pwd. If it
> might trip up other reasers, a comment here would be helpful.
Added a note in V3.
Show 24 quoted lines
> > @@ -21,13 +32,9 @@ test_expect_success 'setup bare repo in worktree' '
> >  	git init --bare outer-repo/bare-repo
> >  '
> >  
> > -test_expect_success 'safe.bareRepository unset' '
> > -	expect_accepted -C outer-repo/bare-repo
> > -'
> > -
> 
> I found this surprising; I thought it was quite valuable to test the
> default behavior. I'd prefer to keep this test.
> 
> Perhaps there was a miscommunication? You mentioned:
> 
>   Actually, explicitly setting the variable here is equivalent to the
>   following test case, so I'll just remove this one.
> 
> which is true, but I think Junio meant _un_setting the variable,
> something like:
> 
>   test_expect_success 'safe.bareRepository unset' '
> +   test_unconfig --global safe.bareRepository &&
>     expect_accepted -C outer-repo/bare-repo
>   '

Yeah, misunderstood. However, see my reply to Junio about this being a "change detector" test.

Josh Steadmon· May 1, 2023, 17:30 UTC · re: Josh Steadmon · lore

[PATCH v3] setup: trace bare repository setups

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.

Range-diff against v2:
1:  b548d96db7 ! 1:  e98be8e7f7 setup: trace bare repository setups
    @@ t/t0035-safe-bare-repository.sh: TEST_PASSES_SANITIZE_LEAK=true
     +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"
     +}
     +
    @@ t/t0035-safe-bare-repository.sh: TEST_PASSES_SANITIZE_LEAK=true
      
      test_expect_success 'setup bare repo in worktree' '
     @@ t/t0035-safe-bare-repository.sh: test_expect_success 'setup bare repo in worktree' '
    - 	git init --bare outer-repo/bare-repo
      '
      
    --test_expect_success 'safe.bareRepository unset' '
    + 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
 setup.c                         |  1 +
 t/t0035-safe-bare-repository.sh | 32 +++++++++++++++++++++++++-------
 2 files changed, 26 insertions(+), 7 deletions(-)
Show changes to 2 files +26 −7

setup.c, t/t0035-safe-bare-repository.sh

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

base-commit: 2807bd2c10606e590886543afe4e4f208dddd489
-- 
2.40.1.495.gc816e09b53d-goog
Junio C Hamano· May 5, 2023, 22:30 UTC · re: Josh Steadmon · lore

Re: [PATCH v3] setup: trace bare repository setups

Josh Steadmon <steadmon@google.com> writes:
Show 34 quoted lines
> 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.

Taylor Blau· May 8, 2023, 22:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] setup: trace bare repository setups

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

Josh Steadmon· May 10, 2023, 23:29 UTC · re: Taylor Blau · lore

Re: [PATCH v3] setup: trace bare repository setups

On 2023.05.08 18:31, Taylor Blau wrote:
Show 13 quoted lines
> 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.

Show 10 quoted lines
>     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.

← back to recent threads