Re: [PATCH v2 0/3] safe.directory clean-up

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Junio

Thanks for picking this up, I think this looks like a good approach 
apart from resolving relative entries in safe.directory which is 
intrinsically unsafe as we don't know which directory the user wants to 
consider safe. With these changes it should not be necessary to add "." 
to safe.directory to get "git daemon" to work and all the other code 
paths use an absolute path from getcwd() so I don't think there is any 
need to support relative directories.

I'll be off the list for the next couple of weeks

Best Wishes

Phillip

On 23/07/2024 03:18, Junio C Hamano wrote:
> Changes since v1:
> 
>   - two "extra" patches for optimization that are unnecessary have
>     been dropped.
>   - a patch to add test for safe.directory="." use case has been
>     added.
> 
> Recently we discussed what we should do when either the path
> configured in the safe.directory configuration or coming from
> the caller of ensure_valid_ownership() function as a result of
> repository discovery is not normalized and textual equality check is
> not sufficient.  See the thread the contains
> 
>    https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com/
> 
> Here are three patches that implement the comparison between
> normalized path and configuration value.
> 
> Imagine that you have a repository at /mnt/disk4/repos/frotz
> directory but in order to make it simpler to manage and use, you
> have your users use /projects/frotz to access the repository.  A
> symlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory
> allows you to do so.
> 
>   - The first patch normalizes the path to the directory that we
>     suspect is a usable repository, before comparing it with the
>     safe.directory configuration variable.  The safe.directory may
>     say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to
>     the repository for the users may be /mnt/disk4/repos/frotz or
>     /projects/frotz, depending on where they come from and what their
>     $PWD makes getcwd() to say.
> 
>   - The second patch normalizes the value of the safe.directory
>     variable.  This allows safe.directory to say /projects/frotz
>     or /projects/* and have them match /mnt/disk4/repos/frotz (which
>     is how the first patch normalizes the repository path to).
> 
>   - The third patch only adds a test to illustrate what happens when
>     the safe.directory configuration is set to ".", which was a
>     plausible setting for those who run "git daemon".  The
>     normalization done by the first two patches is sufficient to make
>     this just work.
> 
> Junio C Hamano (3):
>    safe.directory: normalize the checked path
>    safe.directory: normalize the configured path
>    safe.directory: setting safe.directory="." allows the "current"
>      directory
> 
>   setup.c                   |  28 ++++++--
>   t/t0033-safe-directory.sh | 146 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 170 insertions(+), 4 deletions(-)
> 
> Range-diff against v1:
> 1:  20e1160946 ! 1:  86d5c83ee7 safe.directory: normalize the checked path
>      @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		git config --global safe.directory "$(pwd)/repository"
>       +	) &&
>      -+	git -C repository/ for-each-ref >/dev/null &&
>      -+	git -C repo/ for-each-ref
>      ++	git -C repository for-each-ref &&
>      ++	git -C repository/ for-each-ref &&
>      ++	git -C repo for-each-ref &&
>      ++	git -C repo/ for-each-ref &&
>      ++	test_must_fail git -C repository/.git for-each-ref &&
>      ++	test_must_fail git -C repository/.git/ for-each-ref &&
>      ++	test_must_fail git -C repo/.git for-each-ref &&
>      ++	test_must_fail git -C repo/.git/ for-each-ref
>       +'
>       +
>       +test_expect_success SYMLINKS 'checked leading paths are normalized' '
>      @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		git config --global safe.directory "$(pwd)/repository/*"
>       +	) &&
>      -+	git -C repository/s for-each-ref >/dev/null &&
>      -+	git -C repo/s for-each-ref
>      ++	git -C repository/s for-each-ref &&
>      ++	git -C repository/s/ for-each-ref &&
>      ++	git -C repo/s for-each-ref &&
>      ++	git -C repo/s/ for-each-ref &&
>      ++	git -C repository/s/.git for-each-ref &&
>      ++	git -C repository/s/.git/ for-each-ref &&
>      ++	git -C repo/s/.git for-each-ref &&
>      ++	git -C repo/s/.git/ for-each-ref
>       +'
>       +
>        test_done
> 2:  06de9038b7 ! 2:  c4998076da safe.directory: normalize the configured path
>      @@ setup.c: static int safe_directory_cb(const char *key, const char *value,
>       
>        ## t/t0033-safe-directory.sh ##
>       @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths are normalized' '
>      - 	git -C repo/s for-each-ref
>      + 	git -C repo/s/.git/ for-each-ref
>        '
>        
>       +test_expect_success SYMLINKS 'configured paths are normalized' '
>      @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		git config --global safe.directory "$(pwd)/repo"
>       +	) &&
>      -+	git -C repository/ for-each-ref >/dev/null &&
>      -+	git -C repo/ for-each-ref
>      ++	git -C repository for-each-ref &&
>      ++	git -C repository/ for-each-ref &&
>      ++	git -C repo for-each-ref &&
>      ++	git -C repo/ for-each-ref &&
>      ++	test_must_fail git -C repository/.git for-each-ref &&
>      ++	test_must_fail git -C repository/.git/ for-each-ref &&
>      ++	test_must_fail git -C repo/.git for-each-ref &&
>      ++	test_must_fail git -C repo/.git/ for-each-ref
>       +'
>       +
>       +test_expect_success SYMLINKS 'configured leading paths are normalized' '
>      @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
>       +	git init repository/s &&
>       +	ln -s repository repo &&
>       +	(
>      -+		cd repository &&
>      ++		cd repository/s &&
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		test_commit sample
>       +	) &&
>      @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a
>       +		sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>       +		git config --global safe.directory "$(pwd)/repo/*"
>       +	) &&
>      -+	git -C repository/s for-each-ref >/dev/null &&
>      -+	git -C repo/s for-each-ref
>      ++	git -C repository/s for-each-ref &&
>      ++	git -C repository/s/ for-each-ref &&
>      ++	git -C repository/s/.git for-each-ref &&
>      ++	git -C repository/s/.git/ for-each-ref &&
>      ++	git -C repo/s for-each-ref &&
>      ++	git -C repo/s/ for-each-ref &&
>      ++	git -C repo/s/.git for-each-ref &&
>      ++	git -C repo/s/.git/ for-each-ref
>       +'
>       +
>        test_done
> -:  ---------- > 3:  ffc3f7364e safe.directory: setting safe.directory="." allows the "current" directory