diff mbox series

[v2] setup: trace bare repository setups

Message ID b548d96db7052dc2cb664922fa2003fe068843cf.1682702058.git.steadmon@google.com (mailing list archive)
State Superseded
Headers show
Series [v2] setup: trace bare repository setups | expand

Commit Message

Josh Steadmon April 28, 2023, 5:22 p.m. UTC 
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(-)


base-commit: 2807bd2c10606e590886543afe4e4f208dddd489

Comments

Glen Choo April 28, 2023, 6:37 p.m. UTC | #1 
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
  '
Josh Steadmon May 1, 2023, 5:22 p.m. UTC | #2 
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.
diff mbox series

Patch

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