git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Jul 25, 2024, 09:45 UTC
Message-ID
<d9d1115a-d530-479a-acf8-189713632adf@gmail.com>
In-Reply-To
<20240723021900.388020-1-gitster@pobox.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:
Show 143 quoted lines
> 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
Previous: Junio C HamanoNext: Junio C Hamano
Message 21 of 39 in “safe.directory clean-up”
  1. 0/2 safe.directory clean-upJunio C Hamano, Jul 20, 2024
  2. 1/2 safe.directory: normalize the checked pathJunio C Hamano, Jul 20, 2024
  3. 2/2 safe.directory: normalize the configured pathJunio C Hamano, Jul 20, 2024
  4. 3/2 setup: use a single return path in setup_git_directory*()Junio C Hamano, Jul 20, 2024
  5. 4/2 setup: cache normalized safe.directory configurationJunio C Hamano, Jul 20, 2024
  6. 0/3 safe.directory clean-upJunio C Hamano, Jul 23, 2024
  7. 1/3 safe.directory: normalize the checked pathJunio C Hamano, Jul 23, 2024
  8. 2/3 safe.directory: normalize the configured pathJunio C Hamano, Jul 23, 2024
  9. Phillip WoodJul 25, 2024
  10. Junio C HamanoJul 25, 2024
  11. Phillip WoodAug 14, 2024
  12. Junio C HamanoAug 14, 2024
  13. Phillip WoodAug 15, 2024
  14. Junio C HamanoAug 15, 2024
  15. Jeff KingJul 26, 2024
  16. Junio C HamanoJul 26, 2024
  17. Jeff KingJul 27, 2024
  18. 3/3 safe.directory: setting safe.directory="." allows the "current" directoryJunio C Hamano, Jul 23, 2024
  19. Phillip WoodJul 25, 2024
  20. Junio C HamanoJul 25, 2024
  21. Phillip WoodJul 25, 2024
  22. Junio C HamanoJul 25, 2024
  23. 0/3 safe.directory clean-upJunio C Hamano, Jul 30, 2024
  24. 1/3 safe.directory: normalize the checked pathJunio C Hamano, Jul 30, 2024
  25. 2/3 safe.directory: normalize the configured pathJunio C Hamano, Jul 30, 2024
  26. Jeff KingJul 30, 2024
  27. Junio C HamanoJul 30, 2024
  28. Jeff KingJul 30, 2024
  29. Jeff KingJul 30, 2024
  30. Junio C HamanoJul 30, 2024
  31. safe.directory: preliminary clean-upJunio C Hamano, Jul 30, 2024
  32. Jeff KingJul 30, 2024
  33. Jeff KingJul 30, 2024
  34. 3/3 safe.directory: setting safe.directory="." allows the "current" directoryJunio C Hamano, Jul 30, 2024
  35. 0/4 safe.directory clean-upJunio C Hamano, Jul 30, 2024
  36. 2/4 safe.directory: normalize the checked pathJunio C Hamano, Jul 30, 2024
  37. 1/4 safe.directory: preliminary clean-upJunio C Hamano, Jul 30, 2024
  38. 4/4 safe.directory: setting safe.directory="." allows the "current" directoryJunio C Hamano, Jul 30, 2024
  39. 3/4 safe.directory: normalize the configured pathJunio C Hamano, Jul 30, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.