[PATCH] setup: trace bare repository setups

[PATCH] setup: trace bare repository setups

[Josh Steadmon]

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(-)

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

Re: [PATCH] setup: trace bare repository setups

[Junio C Hamano]

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.

Re: [PATCH] setup: trace bare repository setups

[Glen Choo]

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.

Re: [PATCH] setup: trace bare repository setups

[Josh Steadmon]

On 2023.04.27 16:36, Glen Choo wrote:
> 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.

Re: [PATCH] setup: trace bare repository setups

[Josh Steadmon]

On 2023.04.27 15:54, Junio C Hamano wrote:
> 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!

Re: [PATCH] setup: trace bare repository setups

[Josh Steadmon]

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.

[PATCH v2] setup: trace bare repository setups

[Josh Steadmon]

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(-)

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

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

[Glen Choo]

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.

> @@ -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
  '

Re: [PATCH] setup: trace bare repository setups

[Junio C Hamano]

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?

Re: [PATCH] setup: trace bare repository setups

[Josh Steadmon]

On 2023.04.28 13:26, Junio C Hamano wrote:
> 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? 

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

[Josh Steadmon]

On 2023.04.28 11:37, Glen Choo wrote:
> 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.


> > @@ -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.

[PATCH v3] setup: trace bare repository setups

[Josh Steadmon]

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(-)

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

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

[Junio C Hamano]

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.

Re: [PATCH] setup: trace bare repository setups

[Glen Choo]

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.

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

[Taylor Blau]

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

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

[Josh Steadmon]

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.